Re: [PATCH] Allow breaking out of hung connection attempts - Mailing list pgsql-hackers

On 13.07.2012 14:35, Ryan Kelly wrote:
> On Mon, Jul 09, 2012 at 05:35:15PM +0900, Shigeru HANADA wrote:
>> On Mon, Jun 25, 2012 at 9:00 PM, Ryan Kelly<rpkelly22@gmail.com>  wrote:
>>>> - 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.

poll() potentially performs better, but that hardly matters here, so 
select() is ok.

However, on some platforms, signals don't interrupt select(). On such 
platforms, hitting CTRL-C will still not interrupt the connection 
attempt. Also there's a race condition: if you hit CTRL-C just after 
checking that the global variable is not set, but before entering 
select(), the select() will again not be interrupted (until the user 
gets frustrated and hits CTRL-C again).

I think we need to sleep one second at a time, and check the global 
variable on every iteration, until the timeout is reached. That's what 
we do in non-critical sleep loops like this in the backend. We've 
actually been trying to get rid of such polling in the backend code 
lately, to reduce the number of unnecessary interrupts which consume 
power on an otherwise idle system, but I think that technique would 
still be OK for psql's connection code.

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

Thanks!

With this patch, any connect_timeout parameter given in the original 
connection info string is copied to the \connect command. That sounds 
reasonable at first glance, but I don't think it's quite right:

First of all, if you give a new connect_timeout string directly in the 
\connect command, the old connect_timeout still overrides the new one:

$ ./psql postgres://localhost/postgres?connect_timeout=3
psql (9.3devel)
Type "help" for help.

postgres=# \connect postgres://192.168.1.123/postgres?connect_timeout=1000
<after three seconds>
timeout expired
Previous connection kept

Secondly, the docs say that the new connection only inherits a few 
explicitly listed options from the old connection: dbname, username, 
host and port. If you add connect_timeout to that list, at least it 
requires a documentation update. But I don't think it should be 
inherited in the first place. Or if it should, what about other options, 
like application_name? Surely they should then be inherited too. All in 
all I think we should just leave out the changes to \connect, and not 
inherit connect_timeout from the old connection. If we want to change 
the behavior of all options, that's separate discussion and a separate 
patch.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com