Shigeru:
Thank you very much for your review. Comments are inline below, and a
new patch is attached.
On Fri, Jun 22, 2012 at 10:06:53AM +0900, Shigeru HANADA wrote:
> (2012/05/01 0:30), Ryan Kelly wrote:
> >On Mon, Apr 30, 2012 at 09:02:33AM -0400, Alvaro Herrera wrote:
> >>Well, do *you* want it?
> >Of course. That way I can stop patching my psql and go back to using the
> >one that came with my release :)
>
> Here is result of my review of v4 patch.
>
> Submission
> ----------
> - The patch is in context diff format
> - It needs rebase for HEAD, but patch command takes care automatically.
> - Make finishes cleanly, and all regression tests pass
>
> Functionality
> -------------
> After applying this patch, I could cancel connection attempt at
> start-up by pressing Ctrl+C on terminal just after invoking psql
> command with an unused IP address. Without this patch, such attempt
> ends up with error such as "No route to host" after uninterruptable
> few seconds (maybe the duration until error would depend on
> environment).
>
> 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.
> Detail of changes
> -----------------
> Basically this patch consists of three changes:
>
> 1) defer setup of cancel handler until start-up connection has established
> 2) new libpq API PQconnectTimeout which returns connect_timeout
> value of current session
> 3) use asynchronous connection API at \connect psql command, this
> requires API added by 2)
>
> These seem reasonable to achieve canceling connection attempt at
> start-up and \connect, but, as Ryan says, codes added to do_connect
> might need more simplification.
>
> I have some random comments.
>
> - Checking status by calling PQstatus just after
> PQconnectStartParams is necessary.
Yes, I agree.
> - 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.
> - 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.
>
> Regards,
> --
> Shigeru HANADA
-Ryan Kelly
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5c5dd68..79f4bc0 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1496,6 +1496,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 0926786..23b0106 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,7 +1539,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));
@@ -1557,30 +1557,140 @@ 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)
- {
- PQfinish(n_conn);
- password = prompt_for_password(user);
- continue;
+ if (PQstatus(n_conn) != CONNECTION_BAD) {
+
+ 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;
+
+ /*
+ * 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 +1700,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 +1709,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 7c9fa34..556141c 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
