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 <[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
