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
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 72c9384..0ee2df1 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1375,6 +1375,24 @@ char *PQoptions(const PGconn *conn); </para> </listitem> </varlistentry> + + <varlistentry id="libpq-pqconnecttimeout"> + <term> + <function>PQconnectTimeout</function> + <indexterm> + <primary>PQconnectTimeout</primary> + </indexterm> + </term> + + <listitem> + <para> + Returns the connect_timeout property as given to libpq. +<synopsis> +char *PQconnectTimeout(const PGconn *conn); +</synopsis> + </para> + </listitem> + </varlistentry> </variablelist> </para> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 69fac83..7ba22d0 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1516,7 +1516,7 @@ static bool do_connect(char *dbname, char *user, char *host, char *port) { PGconn *o_conn = pset.db, - *n_conn; + *n_conn = NULL; char *password = NULL; if (!dbname) @@ -1549,7 +1549,7 @@ do_connect(char *dbname, char *user, char *host, char *port) while (true) { -#define PARAMS_ARRAY_SIZE 8 +#define PARAMS_ARRAY_SIZE 9 const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords)); const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values)); @@ -1567,17 +1567,120 @@ do_connect(char *dbname, char *user, char *host, char *port) values[5] = pset.progname; keywords[6] = "client_encoding"; values[6] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto"; - keywords[7] = NULL; - values[7] = NULL; + keywords[7] = "connect_timeout"; + values[7] = PQconnectTimeout(o_conn); + keywords[8] = NULL; + values[8] = NULL; - n_conn = PQconnectdbParams(keywords, values, true); + /* attempt connection asynchronously */ + n_conn = PQconnectStartParams(keywords, values, true); + + if (sigsetjmp(sigint_interrupt_jmp, 1) != 0) + { + /* interrupted during connection attempt */ + PQfinish(n_conn); + n_conn = NULL; + } + else + { + time_t end_time = -1; + + /* + * maybe use a connection timeout. this code essentially stolen + * from src/interfaces/libpq/fe-connect.c connectDBComplete + */ + if (PQconnectTimeout(n_conn) != NULL) + { + int timeout = atoi(PQconnectTimeout(n_conn)); + if (timeout > 0) + { + /* + * Rounding could cause connection to fail; need at least 2 secs + */ + if (timeout < 2) + timeout = 2; + /* calculate the finish time based on start + timeout */ + end_time = time(NULL) + timeout; + } + } + + while(end_time < 0 || time(NULL) < end_time) + { + int poll_res; + int rc; + fd_set read_mask, + write_mask; + struct timeval timeout; + struct timeval *ptr_timeout; + + poll_res = PQconnectPoll(n_conn); + if (poll_res == PGRES_POLLING_OK || + poll_res == PGRES_POLLING_FAILED) + { + break; + } + + if (poll_res == PGRES_POLLING_READING) + FD_SET(PQsocket(n_conn), &read_mask); + if (poll_res == PGRES_POLLING_WRITING) + FD_SET(PQsocket(n_conn), &write_mask); + + /* + * Compute appropriate timeout interval. essentially stolen + * from src/interfaces/libpq/fe-misc.c pqSocketPoll. Maybe + * that function could be made public? we could then replace + * the whole inside of this while loop, assuming it is safe + * to longjmp out from there. + */ + if (end_time == ((time_t) -1)) + ptr_timeout = NULL; + else + { + time_t now = time(NULL); + + if (end_time > now) + timeout.tv_sec = end_time - now; + else + timeout.tv_sec = 0; + timeout.tv_usec = 0; + ptr_timeout = &timeout; + } + + sigint_interrupt_enabled = true; + if (cancel_pressed) + { + PQfinish(n_conn); + n_conn = NULL; + sigint_interrupt_enabled = false; + break; + } + rc = select(PQsocket(n_conn) + 1, + &read_mask, &write_mask, NULL, + ptr_timeout); + sigint_interrupt_enabled = false; + + if (rc < 0 && errno != EINTR) + break; + } + + if (PQstatus(n_conn) != CONNECTION_OK && + end_time > 0 && time(NULL) >= end_time) + { + PQfinish(n_conn); + n_conn = NULL; + fputs(_("timeout expired\n"), stderr); + } + } free(keywords); free(values); /* We can immediately discard the password -- no longer needed */ if (password) + { free(password); + password = NULL; + } if (PQstatus(n_conn) == CONNECTION_OK) break; @@ -1586,7 +1689,7 @@ do_connect(char *dbname, char *user, char *host, char *port) * Connection attempt failed; either retry the connection attempt with * a new password, or give up. */ - if (!password && PQconnectionNeedsPassword(n_conn) && pset.getPassword != TRI_NO) + if (PQconnectionNeedsPassword(n_conn) && pset.getPassword != TRI_NO) { PQfinish(n_conn); password = prompt_for_password(user); @@ -1600,7 +1703,8 @@ do_connect(char *dbname, char *user, char *host, char *port) */ if (pset.cur_cmd_interactive) { - psql_error("%s", PQerrorMessage(n_conn)); + if (n_conn) + psql_error("%s", PQerrorMessage(n_conn)); /* pset.db is left unmodified */ if (o_conn) @@ -1608,7 +1712,9 @@ do_connect(char *dbname, char *user, char *host, char *port) } else { - psql_error("\\connect: %s", PQerrorMessage(n_conn)); + if (n_conn) + psql_error("\\connect: %s", PQerrorMessage(n_conn)); + if (o_conn) { PQfinish(o_conn); diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c index 8b1864c..e53d84c 100644 --- a/src/bin/psql/startup.c +++ b/src/bin/psql/startup.c @@ -111,8 +111,6 @@ main(int argc, char *argv[]) setvbuf(stderr, NULL, _IONBF, 0); #endif - setup_cancel_handler(); - pset.progname = get_progname(argv[0]); pset.db = NULL; @@ -245,8 +243,9 @@ main(int argc, char *argv[]) } /* - * Now find something to do + * Now find something to do (and handle cancellation, if applicable) */ + setup_cancel_handler(); /* * process file given by -f diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index 1af8df6..a5a2fbb 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -160,3 +160,4 @@ PQconnectStartParams 157 PQping 158 PQpingParams 159 PQlibVersion 160 +PQconnectTimeout 161 diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index d454538..e283d07 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -4542,6 +4542,14 @@ PQoptions(const PGconn *conn) return conn->pgoptions; } +char * +PQconnectTimeout(const PGconn *conn) +{ + if (!conn) + return NULL; + return conn->connect_timeout; +} + ConnStatusType PQstatus(const PGconn *conn) { diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index ef26ab9..f2e389e 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -294,6 +294,7 @@ extern char *PQhost(const PGconn *conn); extern char *PQport(const PGconn *conn); extern char *PQtty(const PGconn *conn); extern char *PQoptions(const PGconn *conn); +extern char *PQconnectTimeout(const PGconn *conn); extern ConnStatusType PQstatus(const PGconn *conn); extern PGTransactionStatusType PQtransactionStatus(const PGconn *conn); extern const char *PQparameterStatus(const PGconn *conn,
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers