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