psql sets up cancel handler very early

Lists: pgsql-hackers
From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: psql sets up cancel handler very early
Date: 2013-05-14 15:35:36
Message-ID: 519259C8.6060401@gmx.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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?

Attachment Content-Type Size
psql-startup-cancel-handler.patch text/plain 568 bytes

From: Ryan Kelly <rpkelly22(at)gmail(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: psql sets up cancel handler very early
Date: 2013-05-14 15:41:49
Message-ID: 20130514154149.GD15128@llserver.lakeliving.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I submitted essentially this same patch over a year ago and Tom vetoed
it: http://www.postgresql.org/message-id/3741.1325731017@sss.pgh.pa.us

The thread moved to -hackers at some point and I made some further
enhancements: http://www.postgresql.org/message-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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


From: Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: psql sets up cancel handler very early
Date: 2013-09-18 11:45:24
Message-ID: CAEZATCUifDD9HBH-49TBKmJ9k6_D7Occ8B+SV1=f5JhggRsSjA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 14 May 2013 16:35, Peter Eisentraut <peter_e(at)gmx(dot)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