Thread: [PATCH] Fix Ctrl-C related issues in psql (not for 8.1)

[Please CC any replies to me, thanks. Also, the actual discussion took
place on -hackers, so check there first]

As anyone who has been following -hackers knows, there's been a bit of
discussion about how psql should deal with ^C and pagers. Attached is a
patch that deals with all the memory leak and descriptor leak issues.
With this patch, even valgrind can't find any leaked memory (except a
few things from program startup). With this patch, I find psql a
pleasure to use, it doesn't bug me at all anymore.

As for the apparent complexity of the patch, I'll quote the new comment
from common.c.

 * About ctrlc_abort_jmp and ctrlc_abort_active:
 *
 * In the past the SIGINT handler, when it couldn't find a query to cancel,
 * would simply longjmp() back to the main loop. Very simple but
 * unfortunatly it leaked memory like crazy and file descriptors too if you
 * were using a pager. Hence, the handler has been changed to just set a
 * flag in those cases so the code would notice and gracefully quit.
 *
 * However, PostgreSQL uses signals with BSD semantics, which means the
 * system calls get restarted across a signal. Hence when sitting at the
 * prompt pressing Ctrl-C appeared to do nothing because the kernel would
 * keep restarting the read(). The only way to avoid this is to have the
 * signal handler longjmp().
 *
 * Hence the rules are as follows: If you are reading from a descriptor
 * which may block indefinitly and you want to be able to respond to Ctrl-C
 * you should setup with sigsetjmp() the jump buffer ctrlc_abort_jmp. And
 * when you are about to read something set ctrlc_abort_active to true.
 * After the read set it back to false. If while the flag is true the user
 * presses Ctrl-C, control will pass to the sigsetjmp() block which allows
 * you to clean up gracefully and return. See gets_basic() and
 * gets_readline() in input.c for examples.

Yes, SysV semantics for signals would simplify the code a bit, but I
have no idea whether that's supported or not. Certainly there isn't any
support for it now in the postgres codebase.

There are minor cosmetic changes, like a message when you interrupted
the processing of a query. If you interrupt a pager which dies on ^C
(like more), psql stops generating output straight away instead of
continuing.

I've tested as much as I can think of and it handles everything nicely.
The question of ignoring SIGINT while the pager is active is
orthoginal, this patch doesn't address that at all.

Any comments on coding style and/or side-effects of this patch, don't
hesitate to forward to me.

Also available at:
http://svana.org/kleptog/pgsql/psql-ctrlc.patch

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout <kleptog@svana.org> writes:
> As anyone who has been following -hackers knows, there's been a bit of
> discussion about how psql should deal with ^C and pagers. Attached is a
> patch that deals with all the memory leak and descriptor leak issues.
> With this patch, even valgrind can't find any leaked memory (except a
> few things from program startup). With this patch, I find psql a
> pleasure to use, it doesn't bug me at all anymore.

I've applied a modified form of this patch that also tries to clean up
the behavior for COPY and large-object operations.  Please redo whatever
testing you did before and make sure I didn't break it.

I'm not totally satisfied with the large-object fix yet; sometimes you
have to hit control-C quite a few times before you manage to abort a
long-running \lo_import or \lo_export.  This is because the QueryCancel
request does nothing if it arrives at the backend between lowrite() or
loread() function calls.  The only obvious fix is to duplicate libpq's
lo_import() and lo_export() functions into psql so that we can add
cancel_pressed checks into their loops (obviously libpq itself can't
check that).  Which is kinda yucky.  Any thoughts?

            regards, tom lane

On Wed, Jun 14, 2006 at 12:55:10PM -0400, Tom Lane wrote:
> I'm not totally satisfied with the large-object fix yet; sometimes you
> have to hit control-C quite a few times before you manage to abort a
> long-running \lo_import or \lo_export.  This is because the QueryCancel
> request does nothing if it arrives at the backend between lowrite() or
> loread() function calls.  The only obvious fix is to duplicate libpq's
> lo_import() and lo_export() functions into psql so that we can add
> cancel_pressed checks into their loops (obviously libpq itself can't
> check that).  Which is kinda yucky.  Any thoughts?

Well, the obvious thing I can think of is that PQcancel also set a flag
in the PGconn structure which long-running functions (like lo_import)
in libpq could check.

If lo_import and lo_export are the only two functions that check that
flag, only they need to reset it. Although it probably wouldn't hurt to
reset it each query. I wouldn't worry about using it anywhere else in
libpq, since people seem to be ok with the current semantics.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog@svana.org> writes:
> On Wed, Jun 14, 2006 at 12:55:10PM -0400, Tom Lane wrote:
>> I'm not totally satisfied with the large-object fix yet; sometimes you
>> have to hit control-C quite a few times before you manage to abort a
>> long-running \lo_import or \lo_export.

> Well, the obvious thing I can think of is that PQcancel also set a flag
> in the PGconn structure which long-running functions (like lo_import)
> in libpq could check.

Interesting thought, but PQcancel doesn't actually have access to the
PGconn in the current scheme of things --- it's got a separate struct
in the name of thread safety.  I'm unsure if it'd be safe to let a
PGcancel struct hang onto a link to the PGconn it was made from ...
but I kinda think not.  AFAIR one of the fears that led us to invent the
separate PGcancel struct was the idea that one thread might delete the
PGconn at about the same time another one was trying to issue PQcancel.

            regards, tom lane