On Mon, Jul 09, 2012 at 05:35:15PM +0900, Shigeru HANADA wrote: > Hi Ryan, > > On Mon, Jun 25, 2012 at 9:00 PM, Ryan Kelly <rpkell...@gmail.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. Yes, I had lost them somewhere. My apologies.
> >> - 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. > > http://archives.postgresql.org/message-id/CA+TgmoYZiayts=fjsytzqlg7rewlwkdkey5f+fhp5v5_nu_...@mail.gmail.com I have done this as well. > - 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 I have corrected these issues. > 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. > Regards, > -- > Shigeru Hanada > > * 不明 - 自動検出 > * 英語 > * 日本語 > > * 英語 > * 日本語 > > <javascript:void(0);> -Ryan Kelly
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 5c5dd68..654f5f5 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -436,8 +436,12 @@ switch(PQstatus(conn)) The <literal>connect_timeout</literal> connection parameter is ignored when using <function>PQconnectPoll</function>; it is the application's responsibility to decide whether an excessive amount of time has elapsed. - Otherwise, <function>PQconnectStart</function> followed by a - <function>PQconnectPoll</function> loop is equivalent to + It is recommended to use <function>PQconnectTimeout</function> to get the + value of <literal>connect_timeout</literal> and use that as the timeout + in the application. That way the user gets the same timeout behavior + regardless of whether the application uses <function>PQconnectPoll</function> + or the blocking connection API. Otherwise, <function>PQconnectStart</function> + followed by a <function>PQconnectPoll</function> loop is equivalent to <function>PQconnectdb</function>. </para> @@ -1496,6 +1500,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 c5ec981..d987be8 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1506,7 +1506,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) @@ -1539,9 +1539,9 @@ do_connect(char *dbname, char *user, char *host, char *port) while (true) { -#define PARAMS_ARRAY_SIZE 8 - const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords)); - const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values)); +#define PARAMS_ARRAY_SIZE 9 + const char *keywords[PARAMS_ARRAY_SIZE]; + const char *values[PARAMS_ARRAY_SIZE]; keywords[0] = "host"; values[0] = host; @@ -1557,30 +1557,145 @@ 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); - - free(keywords); - free(values); - - /* We can immediately discard the password -- no longer needed */ - if (password) - free(password); - - if (PQstatus(n_conn) == CONNECTION_OK) - break; + /* attempt connection asynchronously */ + n_conn = PQconnectStartParams(keywords, values, true); /* - * Connection attempt failed; either retry the connection attempt with - * a new password, or give up. + * if we get CONNECTION_BAD, PQconnectStartParams has failed, so don't + * bother even attempting to connect. */ - if (!password && PQconnectionNeedsPassword(n_conn) && pset.getPassword != TRI_NO) + if (PQstatus(n_conn) != CONNECTION_BAD) { - PQfinish(n_conn); - password = prompt_for_password(user); - continue; + 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; + } + + FD_ZERO(&read_mask); + FD_ZERO(&write_mask); + + 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); + } + } + + /* We can immediately discard the password -- no longer needed */ + if (password) + { + free(password); + password = NULL; + } + + if (PQstatus(n_conn) == CONNECTION_OK) + break; + + /* + * Connection attempt failed; either retry the connection attempt + * with a new password, or give up. + */ + if (PQconnectionNeedsPassword(n_conn) && pset.getPassword != TRI_NO) + { + PQfinish(n_conn); + password = prompt_for_password(user); + continue; + } } /* @@ -1590,7 +1705,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) @@ -1598,7 +1714,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 9a6306b..c3ffea5 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; @@ -253,8 +251,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 1251455..fe98c7a 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -163,3 +163,4 @@ PQlibVersion 160 PQsetRowProcessor 161 PQgetRowProcessor 162 PQskipResult 163 +PQconnectTimeout 164 diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index a32258a..065b782 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -5161,6 +5161,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 67db611..3ea168d 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -310,6 +310,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