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

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.

>> - 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

- 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

Once the issues above are fixed, IMO this patch can be marked as "Ready
for committer".

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