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

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(at)gmail(dot)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