Re: [PATCH] Allow breaking out of hung connection attempts

On Mon, Jul 09, 2012 at 05:35:15PM +0900, Shigeru HANADA wrote:
> Hi Ryan,
>
> On Mon, Jun 25, 2012 at 9:00 PM, Ryan Kelly <rpkelly22@gmail.com> wrote:
> >> Connection attempt by \connect command could be also canceled by
> >> pressing Ctrl+C on psql prompt.
> >>
> >> In addition, I tried setting PGCONNECT_TIMEOUT to 0 (infinite), but
> >> psql gave up after few seconds, for both start-up and re-connect.
> >> Is this intentional behavior?
> > A timeout of 0 (infinite) means to keep trying until we succeed or fail,
> > not keep trying forever. As you mentioned above, your connection
> > attempts error out after a few seconds. This is what is happening. In my
> > environment no such error occurs and as a result psql continues to
> > attempt to connect for as long as I let it.
>
> For handy testing, I wrote simple TCP server which accepts connection
> and blocks until client gives up connection attempt (or forever).  When
> I tested your patch with setting PGCONNECT_TIMEOUT=0, I found that
> psql's CPU usage goes up to almost 100% (10~ usr + 80~ sys) while
> connection attempt by \c command is undergoing.  After reading code for
> a while, I found that FD_ZERO was not called.  This makes select() to
> return immediately in the loop every time and caused busy loop.
> # Maybe you've forgotten adding FD_ZERO when you were merging Heikki's
> v2 patch.
Yes, I had lost them somewhere. My apologies.

> >> - Checking status by calling PQstatus just after
> >> PQconnectStartParams is necessary.
> > Yes, I agree.
>
> Checked.
>
> >> - Copying only "select()" part of pqSocketPoll seems not enough,
> >> should we use poll(2) if it is supported?
> > I did not think the additional complexity was worth it in this case.
> > Unless you see some reason to use poll(2) that I do not.
>
> I checked where select() is used in PG, and noticed that poll is used at
> only few places.  Agreed to use only select() here.
>
> >> - Don't we need to clear error message stored in PGconn after
> >> PQconnectPoll returns OK status, like connectDBComplete?
> > I do not believe there is a client interface for clearing the error
> > message. Additionally, the documentation states that PQerrorMessage
> > "Returns the error message most recently generated by an operation on
> > the connection." This seems to indicate that the error message should be
> > cleared as this behavior is part of the contract of PQerrorMessage.
>
> My comment was pointless.  Sorry for noise.
>
> Here is my additional comments for v5 patch:
>
> - Using static array for fixed-length connection parameters was
> suggested in comments for another CF item, using
> fallback_application_name for some command line tools, and IMO this
> approach would also suit for this patch.
>
> http://archives.postgresql.org/message-id/CA+TgmoYZiayts=FjSytZqLg7REwLWKDkEY5f+fHP5V5_nu_iiA@mail.gmail.com
I have done this as well.

> - Some comments go past 80 columns, and opening brace in line 1572
> should go on next line.  Please refer coding conventions written in
> PostgreSQL wiki.
> http://www.postgresql.org/docs/devel/static/source-format.html
I have corrected these issues.

> Once the issues above are fixed, IMO this patch can be marked as "Ready
> for committer".
I have also added additional documentation reflecting Heikki's
suggestion that PQconnectTimeout be recommended for use by applications
using the async API.

Attached is v6 of the patch.

> Regards,
> --
> Shigeru Hanada
>
>   * 不明 - 自動検出
>   * 英語
>   * 日本語
>
>   * 英語
>   * 日本語
>
>  <javascript:void(0);>

-Ryan Kelly