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.
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.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 69fac83..135d022 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -1515,8 +1515,8 @@ param_is_newly_set(const char *old_val, const char *new_val)
static bool
do_connect(char *dbname, char *user, char *host, char *port)
{
- PGconn *o_conn = pset.db,
- *n_conn;
+ PGconn *o_conn = pset.db;
+ PGconn *n_conn = NULL;
char *password = NULL;
if (!dbname)
@@ -1570,14 +1570,67 @@ do_connect(char *dbname, char *user, char *host, char *port)
keywords[7] = NULL;
values[7] = 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
+ {
+ while (true)
+ {
+ int poll_res;
+ int rc;
+ fd_set read_mask,
+ write_mask;
+
+ 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);
+
+ sigint_interrupt_enabled = true;
+ if (cancel_pressed)
+ {
+ /* interrupted during connection attempt */
+ PQfinish(n_conn);
+ n_conn = NULL;
+ sigint_interrupt_enabled = false;
+ break;
+ }
+ rc = select(PQsocket(n_conn) + 1,
+ &read_mask, &write_mask, NULL,
+ NULL);
+ sigint_interrupt_enabled = false;
+
+ if (rc < 0 && errno != EINTR)
+ break;
+ }
+ }
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 +1639,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 +1653,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 +1662,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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
