From: | Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com> |
---|---|
To: | Ryan Kelly <rpkelly22(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Allow breaking out of hung connection attempts |
Date: | 2012-07-09 08:35:15 |
Message-ID: | 4FFA97C3.3070601@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Ryan,
On Mon, Jun 25, 2012 at 9:00 PM, Ryan Kelly <rpkelly22(at)gmail(dot)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.
- 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);>
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2012-07-09 10:30:23 | Re: huge tlb support |
Previous Message | Martijn van Oosterhout | 2012-07-09 07:15:18 | Re: huge tlb support |