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

On Tue, Jan 10, 2012 at 11:29:58AM +0200, Heikki Linnakangas wrote:
> On 09.01.2012 15:49, Ryan Kelly wrote:
> >On Mon, Jan 09, 2012 at 10:35:50AM +0200, Heikki Linnakangas wrote:
> >>That assumes that it's safe to longjmp out of PQconnectdbParams at
> >>any instant. It's not.
> >I'm guessing because it could result in a resource leak?
> 
> Yes, and other unfinished business, too.
> 
> >>I think you'd need to use the asynchronous connection functions
> >>PQconnectStartParams() and PQconnectPoll(), and select().
> >New patch attached.
> 
> Thanks, some comments:
> 
> * Why do you need the timeout?
> 
> * If a SIGINT arrives before you set sigint_interrupt_enabled, it
> just sets cancel_pressed but doesn't jump out of the connection
> attempt. You need to explicitly check cancel_pressed after setting
> sigint_interrupt_enabled to close that race condition.
> 
> * You have to reinitialize the fd mask with FD_ZERO/SET before each
> call to select(). select() modifies the mask.
> 
> * In case of PGRES_POLLING_WRITING, you have to wait until the
> socket becomes writable, not readable.
> 
> Attached is a new version that fixes those.

Yup, I'm an idiot.

> 
> There's one caveat in the libpq docs about PQconnectStart/PQconnectPoll:
> 
> >The connect_timeout connection parameter is ignored when using PQconnectPoll; it is the application's responsibility to decide whether an excessive amount of time has elapsed. Otherwise, PQconnectStart followed by a PQconnectPoll loop is equivalent to PQconnectdb.
> 
> So after this patch, connect_timeout will be ignored in \connect.
> That probably needs to be fixed. You could incorporate a timeout
> fairly easily into the select() calls, but unfortunately there's no
> easy way to get the connect_timeout value. You could to parse the
> connection string the user gave with PQconninfoParse(), but the
> effective timeout setting could come from a configuration file, too.
> 
> Not sure what to do about that. If there was a
> PQconnectTimeout(conn) function, similar to PQuser(conn),
> PQhost(conn) et al, you could use that. Maybe we should add that, or
> even better, a generic function that could be used to return not
> just connect_timeout, but all the connection options in effect in a
> connection.

I have attached a new patch which handles the connect_timeout option by
adding a PQconnectTimeout(conn) function to access the connect_timeout
which I then use to retrieve the existing value from the old connection.

I also "borrowed" some code from other places:

* connectDBComplete had some logic surrounding handling the timeout
  value (src/interfaces/libpq/fe-connect.c:1402).
* The timeout code is lifted from pqSocketPoll which, if made public,
  could essentially replace all the select/timeout code in that loop
  (src/interfaces/libpq/fe-misc.c:1034).

Before I get flamed for duplicating code, yes, I know it's a bad thing
to do. If anyone has any guidance I'd be glad to implement their
suggestions.

-Ryan Kelly