psql sets up cancel handler very early

Started by Peter Eisentrautover 12 years ago3 messages
#1Peter Eisentraut
peter_e@gmx.net
1 attachment(s)

Sometimes, the psql startup hangs when it cannot resolve or connect to a
host. Intuitively, I would like to press Ctrl+C and correct the
connection string or investigate. But that doesn't work because Ctrl+C
is already bound to the query cancel handler by that time.

It seems to me that there is no point in setting up the cancel handler
before the database connection is established. Example patch attached.
Comments?

Attachments:

psql-startup-cancel-handler.patchtext/plain; charset=UTF-8; name=psql-startup-cancel-handler.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5d7fe6e..65a1cde 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -111,8 +111,6 @@ static void parse_psql_options(int argc, char *argv[],
 	setvbuf(stderr, NULL, _IONBF, 0);
 #endif
 
-	setup_cancel_handler();
-
 	pset.progname = get_progname(argv[0]);
 
 	pset.db = NULL;
@@ -249,6 +247,8 @@ static void parse_psql_options(int argc, char *argv[],
 		exit(EXIT_BADCONN);
 	}
 
+	setup_cancel_handler();
+
 	PQsetNoticeProcessor(pset.db, NoticeProcessor, NULL);
 
 	SyncVariables();
#2Ryan Kelly
rpkelly22@gmail.com
In reply to: Peter Eisentraut (#1)
Re: psql sets up cancel handler very early

I submitted essentially this same patch over a year ago and Tom vetoed
it: /messages/by-id/3741.1325731017@sss.pgh.pa.us

The thread moved to -hackers at some point and I made some further
enhancements: /messages/by-id/20120108201802.GA31348@llserver.lakeliving.com

Never went anywhere though.

-Ryan P. Kelly

On Tue, May 05/14/13, 2013 at 11:35:36AM -0400, Peter Eisentraut wrote:

Sometimes, the psql startup hangs when it cannot resolve or connect to a
host. Intuitively, I would like to press Ctrl+C and correct the
connection string or investigate. But that doesn't work because Ctrl+C
is already bound to the query cancel handler by that time.

It seems to me that there is no point in setting up the cancel handler
before the database connection is established. Example patch attached.
Comments?

diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5d7fe6e..65a1cde 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -111,8 +111,6 @@ static void parse_psql_options(int argc, char *argv[],
setvbuf(stderr, NULL, _IONBF, 0);
#endif

- setup_cancel_handler();
-
pset.progname = get_progname(argv[0]);

pset.db = NULL;
@@ -249,6 +247,8 @@ static void parse_psql_options(int argc, char *argv[],
exit(EXIT_BADCONN);
}

+	setup_cancel_handler();
+
PQsetNoticeProcessor(pset.db, NoticeProcessor, NULL);

SyncVariables();

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Peter Eisentraut (#1)
Re: psql sets up cancel handler very early

On 14 May 2013 16:35, Peter Eisentraut <peter_e@gmx.net> wrote:

Sometimes, the psql startup hangs when it cannot resolve or connect to a
host. Intuitively, I would like to press Ctrl+C and correct the
connection string or investigate. But that doesn't work because Ctrl+C
is already bound to the query cancel handler by that time.

It seems to me that there is no point in setting up the cancel handler
before the database connection is established. Example patch attached.
Comments?

That makes sense to me, and the patch appears to work as advertised.

The objections to the previous patch were that it did nothing in the
\c case, or if the server becomes unreachable mid-session. Those feel
like much less common cases, but maybe they're still worth thinking
about. However, IMO the solution to those issues is likely to be a
significantly different (and larger) patch.

Also, even if those issues do get addressed one day, the change in
this patch still seems like the right thing to do on initial startup,
which IME is the most common case, so +1 for this patch.

I'm marking it ready for committer.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers