Question about Ctrl-C and less

Martijn van Oosterhout <kleptog@svana.org> writes:

This problem has been around for ever yet obviously not everybody runs
into it all the time like I do. Would patch to fix this be accepted or
is there a reason why not?

I guess everybody else uses "q" not control-C to get out of less ;-)

I'm not sure I agree with the concept of blocking SIGINT in this context
--- you would not, I trust, propose blocking SIGQUIT or SIGHUP, but then
why is it ok to block SIGINT?

AFAICT it wouldn't fix the problem anyway, as there's still a bug in
less: it doesn't restore the terminal settings on exit.

regards, tom lane

On Sun, Oct 16, 2005 at 02:44:41PM -0400, Tom Lane wrote:

Martijn van Oosterhout <kleptog@svana.org> writes:

This problem has been around for ever yet obviously not everybody runs
into it all the time like I do. Would patch to fix this be accepted or
is there a reason why not?

I guess everybody else uses "q" not control-C to get out of less ;-)

Umm, Ctrl-C doesn't quit less, it aborts the current command. So if
it's doing a search or you've hit "F" (follow) or any number of other
things then the only way out is to press Ctrl-C. You have to press "q"
to quit less like you say.

I'm not sure I agree with the concept of blocking SIGINT in this context
--- you would not, I trust, propose blocking SIGQUIT or SIGHUP, but then
why is it ok to block SIGINT?

The system() function blocks -INT and -QUIT, as does the shell when you
run a program, so why not?

AFAICT it wouldn't fix the problem anyway, as there's still a bug in
less: it doesn't restore the terminal settings on exit.

The point is, less is still running but psql it pulling the keystrokes
from under it... When you finally quit less it does restore the
settings, but now readline doesn't expect that as it's changed them
again...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

The point is, less is still running but psql it pulling the keystrokes
from under it... When you finally quit less it does restore the
settings, but now readline doesn't expect that as it's changed them
again...

Martin,

Let's see the patch. I assume it should be fairly small. If we could get
it in early in the 8.2 cycle we would have plenty of time to bang on it.
In principle this sounds reasonable to me, but psql can be broken quite
easily - I know as I've done it a couple of times ;-)

cheers

andrew

Martjin,

This problem has been around for ever yet obviously not everybody runs
into it all the time like I do. Would patch to fix this be accepted or
is there a reason why not?

Actually, I run into it fairly often when I'm being hasty. I'd imagine most
Linux-based newbies do as well.

--
Josh Berkus
Aglio Database Solutions
San Francisco

On Sun, Oct 16, 2005 at 04:06:04PM -0400, Andrew Dunstan wrote:

Martin,

Let's see the patch. I assume it should be fairly small. If we could get
it in early in the 8.2 cycle we would have plenty of time to bang on it.
In principle this sounds reasonable to me, but psql can be broken quite
easily - I know as I've done it a couple of times ;-)

Very well, patch attached. It's quite simple actually. However, there
seems to be some push back from people suggesting that jumping back to
the main loop before the pager has quit is not buggy behaviour.
Assuming that a ^C will kill the pager is just folly.

Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard
procedure for spawning external interactive processes includes blocking
SIGQUIT too (see system() manpage). Logically speaking, when the user
sends an interrupt from the keyboard they expect to interrupt the
currently active *interaxtive* process. Hence, once psql has spawned
the pager, it should ignore such interrupts until control is returned
(after pclose). So yes, I would suggest blocking SIGQUIT also, if only
to prevent terminal corruption problems. Interactive programs like less
trap SIGQUIT to restore the terminal settings on exit, but the exit
anyway.

SIGHUP is different. It is sent by the system, not the user, indicating
the terminal has disconnected. psql is not a daemon expecting to
survive that and hence should quit as normal, along with all other
processes on that terminal.

If this isn't going to make 8.1 anyway I'd probably go for a patch that
got rid of the longjmp altogether (if people will accept that), fixing
the memory leaks at the same time. At the moment I'm just looking for a
concensus that it's a problem to be solved.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

On Sun, Oct 16, 2005 at 01:57:13PM -0700, Josh Berkus wrote:

Martjin,

This problem has been around for ever yet obviously not everybody runs
into it all the time like I do. Would patch to fix this be accepted or
is there a reason why not?

Actually, I run into it fairly often when I'm being hasty. I'd imagine most
Linux-based newbies do as well.

I believe many such applications (including less?) are following the
Emacs tradition(?) of having Control-C, or often, Control-G (with
Control-G actually being re-bound as the interrupt character) - mean
interrupt the current operation within the application - not cause
the application to exit. If I wanted to quit, I'd hit 'q'. The times
I do hit Control-C, or Control-G, are when the application I believe
I am using (my keystrokes are interacting with) is performing a
lengthy operation, that I wish to interrupt.

One such operation that has come up with me in less, is when it is
attempting to calculate the line numbers. It becomes non-responsive
during this time, and if I don't care what the line number is, I
interrupt less to tell it 'no - don't calculate line numbers - get
ready for my next command'.

So, I'm with Martijn. It's not right for psql to die from SIGINT
during the execution of another command. SIGINT is a user signal
that can be generated from a keyboard. Psql should not be making
assumptions about what the signal is meant to mean, to the current
application in control of the keyboard.

I agree with Martijn on SIGQUIT (another keyboard signal!) being
ignored, but leaving SIGHUP (not a keyboard signal!) cause psql to
exit. SIGHUP is usually sent to everybody, with everybody paying
attention to it. SIGINT and SIGQUIT are intended for the application
in control of the terminal.

Cheers,
mark

--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada

One ring to rule them all, one ring to find them, one ring to bring them all
and in the darkness bind them...

http://mark.mielke.cc/

I run into this problem sometimes, especially when I realize that
the query I've just started is going to run for a very long time and
not really provide anything useful. I find that I have to close the
shell window to get out of it, and I'm always a bit uncomforatble
doing that.

-Kevin

Martijn van Oosterhout <kleptog@svana.org> >>>

At the moment I'm just looking for a
concensus that it's a problem to be solved.

On Tue, Oct 18, 2005 at 05:15:20PM -0500, Kevin Grittner wrote:

I run into this problem sometimes, especially when I realize that
the query I've just started is going to run for a very long time and
not really provide anything useful. I find that I have to close the
shell window to get out of it, and I'm always a bit uncomforatble
doing that.

Hmm, I'm glad it isn't just me experiencing this on a regular basis.
It's possibly also because Debian sets the default pager to less, which
makes it happen on every machine I use.

The simple patch I posted seems unlikely to make it into 8.1 so I'll
add a more comprehensive patch to my list hopefully for 8.2...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

Very well, patch attached. It's quite simple actually. However, there
seems to be some push back from people suggesting that jumping back to
the main loop before the pager has quit is not buggy behaviour.
Assuming that a ^C will kill the pager is just folly.

Making assumptions about what the pager will do upon receipt of SIGINT
is folly as well.

Setting up SIGINT to be ignored may be the right answer (I don't
believe it is -- see below), but if so then it needs to be done
properly. If it gets ignored prior to the popen(), then the child
will also end up ignoring it by default, because signal disposition is
inherited by child processes. If we ignore SIGINT, it should be after
the popen(), not before.

When the user sends SIGINT, he means to interrupt whatever processing
is currently occurring. He expects to regain control of the
terminal. If psql is in the process of sending data to the pager,
then a SIGINT should cause psql to stop doing so.

So I think the right answer here is for psql to handle SIGINT
internally by doing a pclose() first (and at this point, it probably
should ignore SIGINT altogether), then returning to the main loop
(and, of course, cleaning up anything that needs it along the way).
If the child hasn't exited then pclose() will block until it has. The
end result should be the semantics you want: if psql is in the middle
of sending a bunch of rows of output to the pager, this will interrupt
that process. If the pager remains running then it will hopefully
give the user the ability to scroll through whatever results were sent
to it.

Tom asked if we should be blocking SIGQUIT and SIGHUP too. Standard
procedure for spawning external interactive processes includes blocking
SIGQUIT too (see system() manpage).

SIGQUIT has a different standard meaning in Unix than SIGINT: it
causes the process to drop core. We should not be blocking it -- we
should be leaving it alone. The reason is that it's quite possible
that the user wants to have psql generate a core file while it's
writing output to the pager.

Logically speaking, when the user sends an interrupt from the
keyboard they expect to interrupt the currently active *interaxtive*
process.

They expect to interrupt the currently active processing. Not quite
the same thing.

Hence, once psql has spawned
the pager, it should ignore such interrupts until control is returned
(after pclose). So yes, I would suggest blocking SIGQUIT also, if only
to prevent terminal corruption problems. Interactive programs like less
trap SIGQUIT to restore the terminal settings on exit, but the exit
anyway.

They should be dropping core upon receipt of SIGQUIT. It might be
nice if they cleaned up the terminal first, but receipt of a SIGQUIT
generally means that the user wants to run the resulting core file
through a debugger, and trapping the signal could alter the stack such
that the resulting core would be less useful. I'd rather have to
clean up the terminal manually than have an unusable core file.

--
Kevin Brown kevin@sysexperts.com

On Tue, Oct 18, 2005 at 09:32:25PM -0700, Kevin Brown wrote:

So I think the right answer here is for psql to handle SIGINT
internally by doing a pclose() first (and at this point, it probably
should ignore SIGINT altogether), then returning to the main loop
(and, of course, cleaning up anything that needs it along the way).
If the child hasn't exited then pclose() will block until it has. The
end result should be the semantics you want: if psql is in the middle
of sending a bunch of rows of output to the pager, this will interrupt
that process. If the pager remains running then it will hopefully
give the user the ability to scroll through whatever results were sent
to it.

That's what I meant by "more comprehensive patch". Basically, the
longjmp has to go because it leaks memory (and file descriptors) and
doesn't allow you to control things at the right level. My little patch
basically set the signal handler *after* the popen so everything works
as expected.

My plan is to have the interrupt handler set a flag "control_c_pressed"
and check it at strategic points. Then memory can be deallocated and
returned properly. It's a lot more invasive and the corners cases will
be "interesting".

Your point about SIGQUIT is valid. I didn't include it in my patch
since I wasn't sure what the expected behaviour would be. If I got core
files everytime I pressed SIGQUIT, I'd have a lot of core files
scattered around my disk; one of the reasons I disable core files by
default.

OTOH, if I wanted to trap psql, I'd run it under a debugger or attach
one which would catch SIGQUIT even if the program ignores it.

Anyway, thanks for the response, hopefully we can get this sorted out
in a later release...
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

On Tue, Oct 18, 2005 at 09:32:25PM -0700, Kevin Brown wrote:

So I think the right answer here is for psql to handle SIGINT
internally by doing a pclose() first (and at this point, it probably
should ignore SIGINT altogether), then returning to the main loop
(and, of course, cleaning up anything that needs it along the way).
If the child hasn't exited then pclose() will block until it has. The
end result should be the semantics you want: if psql is in the middle
of sending a bunch of rows of output to the pager, this will interrupt
that process. If the pager remains running then it will hopefully
give the user the ability to scroll through whatever results were sent
to it.

That's what I meant by "more comprehensive patch". Basically, the
longjmp has to go because it leaks memory (and file descriptors) and
doesn't allow you to control things at the right level. My little patch
basically set the signal handler *after* the popen so everything works
as expected.

It does? It looked to me like it was setting the signal handler to
SIG_IGN before doing the popen(), and resetting it to the internal
signal handler after doing the pclose().

Am I examining the wrong patch?

My plan is to have the interrupt handler set a flag "control_c_pressed"
and check it at strategic points. Then memory can be deallocated and
returned properly. It's a lot more invasive and the corners cases will
be "interesting".

If I were to take that approach, I'd do the check immediately after
writing to the pipe for sure, and possibly after reading from the
database handle. I'd have to look at the code to see what's going on
and thus where the best places to stick the checks are. But in
general, you want to put a check anywhere within the loop that is
immediately after a blocking operation such as a read or write.
However, that said...

The semantics for handling SIGINT here should be pretty much the same
as they are when there's no pager at all (which I know works because I
can interrupt the output anytime and get the psql prompt back). In
fact, this suggests that the proper course of action would be to store
the pipe's file descriptor someplace accessible to the normal SIGINT
handler, and have the SIGINT handler check its value upon receipt of
SIGINT. If the value is nonzero then pclose() it and then proceed as
before (and zero the descriptor after any pclose() so that the handler
always does the right thing). If there's any additional memory that
has to be freed, then do that too, of course.

That'll take care of the descriptor leak and it'll get you the right
semantics as a nice bonus.

Your point about SIGQUIT is valid. I didn't include it in my patch
since I wasn't sure what the expected behaviour would be. If I got core
files everytime I pressed SIGQUIT, I'd have a lot of core files
scattered around my disk; one of the reasons I disable core files by
default.

If you're hitting SIGQUIT a lot then perhaps you need to assign it to
a different key. :-)

Seriously, you shouldn't be using SIGQUIT unless you want a core file.
Otherwise, use SIGINT. If the application is unresponsive to SIGINT,
my normal course of action is to suspend it via job control signals
(i.e., send SIGTSTP) and then send it a SIGTERM via the kill command.
If that doesn't do it, then it gets a SIGKILL. And if it doesn't stop
upon receipt of SIGTSTP I kill it externally and complain to its
developers (assuming it isn't hung in some uninterruptible sleep in
the kernel or something).

OTOH, if I wanted to trap psql, I'd run it under a debugger or attach
one which would catch SIGQUIT even if the program ignores it.

Suppose it's hung on a really hard to reproduce condition? Point
being that you don't necessarily know ahead of time that you're going
to want a core file. Otherwise SIGQUIT wouldn't have the default
semantics it has, and it wouldn't have a terminal key associated with
it. The guys that designed this stuff knew what they were doing.

--
Kevin Brown kevin@sysexperts.com

On 2005-10-19, Kevin Brown <kevin@sysexperts.com> wrote:

Making assumptions about what the pager will do upon receipt of SIGINT
is folly as well.

Setting up SIGINT to be ignored may be the right answer (I don't
believe it is -- see below), but if so then it needs to be done
properly. If it gets ignored prior to the popen(), then the child
will also end up ignoring it by default, because signal disposition is
inherited by child processes. If we ignore SIGINT, it should be after
the popen(), not before.

I do not believe it is possible to do the signal disposition correctly
and still use popen() to run the pager. (You would need to reimplement
popen using raw syscalls.)

So I think the right answer here is for psql to handle SIGINT
internally by doing a pclose() first

The chances that psql can do this safely approach zero. pclose() is not a
signal-safe function, so it can only be called from a signal handler if
you _know_ that the signal did not interrupt any non-signal-safe function.
(Nor can the signal handler longjmp out in such a case, unless the code is
never again going to call any unsafe function.)

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

On Wed, Oct 19, 2005 at 04:24:21AM -0700, Kevin Brown wrote:

It does? It looked to me like it was setting the signal handler to
SIG_IGN before doing the popen(), and resetting it to the internal
signal handler after doing the pclose().

Oops, you're right. It works because less sets it's own signal handler,
as does more. Sure, the status is inherited but I doubt there's a pager
out there that doesn't set its own signal handlers. Still, best do it
properly.

If I were to take that approach, I'd do the check immediately after
writing to the pipe for sure, and possibly after reading from the
database handle. I'd have to look at the code to see what's going on
and thus where the best places to stick the checks are. But in
general, you want to put a check anywhere within the loop that is
immediately after a blocking operation such as a read or write.
However, that said...

Well, the pager is only run after all the data has been collected from
the server so none of this is an issue while the query is being
processed. Send the cancel request and toss the data. Once we start the
pager just check after each line is printed.

The semantics for handling SIGINT here should be pretty much the same
as they are when there's no pager at all (which I know works because I

<snip>

You can't do a pclose in a signal handler, it's not one of the
"reeentrant safe" functions and could lead to deadlocks. The signal
manpage documents the ones you can use. Just set a flag. Setting the
descriptor to NULL is worse because then we have check before every
output function. fprintf(NULL, ...) will segfault on most
architechtures I wager.

Also, the signal handler can't free the memory, there's way too many
arrays. A flag allows the output functions to exit sanely.

If you're hitting SIGQUIT a lot then perhaps you need to assign it to
a different key. :-)

I use SIGQUIT to get out of the situation you get currently when you
press Ctrl-C to interrupt less. Somehow all the "q"s go to psql and all
the Ctrl-Ds to less. SIGQUIT just kills them both from where I can
clean the terminal up. So I will press SIGQUIT less once we fix up psql
to not do stupid things on SIGINT. :)

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

You can't do a pclose in a signal handler, it's not one of the
"reeentrant safe" functions and could lead to deadlocks. The signal
manpage documents the ones you can use. Just set a flag. Setting the
descriptor to NULL is worse because then we have check before every
output function. fprintf(NULL, ...) will segfault on most
architechtures I wager.

Yeah, I was thinking that you'd do the check for the flag and invoke a
cleanup handler after the write() to the output file descriptor. It's
not clear that you'd need to do the check anyplace else. It's been a
while since I've messed with this stuff, but if I recall correctly,
the write() will return immediately after receipt of a signal, and
will indicate how much was actually written. So receipt of a SIGINT
should wind up being handled in a reasonably timely fashion.

Additionally the normal SIGINT signal handler (the one that gets
invoked when the pager is turned off) can be called from the cleanup
handler in order to maintain the proper semantics.

--
Kevin Brown kevin@sysexperts.com

On Thu, Oct 20, 2005 at 03:42:10PM -0700, Kevin Brown wrote:

Martijn van Oosterhout wrote:

You can't do a pclose in a signal handler, it's not one of the
"reeentrant safe" functions and could lead to deadlocks. The signal
manpage documents the ones you can use. Just set a flag. Setting the
descriptor to NULL is worse because then we have check before every
output function. fprintf(NULL, ...) will segfault on most
architechtures I wager.

Yeah, I was thinking that you'd do the check for the flag and invoke a
cleanup handler after the write() to the output file descriptor. It's
not clear that you'd need to do the check anyplace else. It's been a
while since I've messed with this stuff, but if I recall correctly,
the write() will return immediately after receipt of a signal, and
will indicate how much was actually written. So receipt of a SIGINT
should wind up being handled in a reasonably timely fashion.

Additionally the normal SIGINT signal handler (the one that gets
invoked when the pager is turned off) can be called from the cleanup
handler in order to maintain the proper semantics.

I disagree that psql should make *any* assumptions about what SIGINT
means to the child process. Consider less again, and Control-C used
to abort a search. You are suggesting that Control-C should not only
abort the search, but should also cut off the input from less. Less
won't die. Less will just see a terminated input stream. What has been
gained from this? Is this intuitive behaviour?

If the pager does die in response to SIGINT, the write() will fail with
SIGPIPE. Completely clean, without any need for psql to pay attention
to SIGINT.

I think the only reasonable behaviour is to ignore SIGINT within the
parent, until the child exits. I don't see why other behaviours are
even being considered. To me, it points at a misunderstanding of the
problem.

Cheers,
mark

--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada

One ring to rule them all, one ring to find them, one ring to bring them all
and in the darkness bind them...

http://mark.mielke.cc/

On Thu, Oct 20, 2005 at 08:11:14PM -0400, mark@mark.mielke.cc wrote:

I disagree that psql should make *any* assumptions about what SIGINT
means to the child process. Consider less again, and Control-C used
to abort a search. You are suggesting that Control-C should not only
abort the search, but should also cut off the input from less. Less
won't die. Less will just see a terminated input stream. What has been
gained from this? Is this intuitive behaviour?

I must say I agree with the idea that Ctrl-C shouldn't stop the stream
from psql, but I'm willing to let it slide because a lot of other
programs work this way. I imagine asking it to be configurable will
meet even more resistance.

I think the only reasonable behaviour is to ignore SIGINT within the
parent, until the child exits. I don't see why other behaviours are
even being considered. To me, it points at a misunderstanding of the
problem.

I've been playing with a version of psql which on Ctrl-C doesn't
longjmp() but politely frees everything, waits for the pager and then
back to the main loop with the message "Interrupted". But now we have
another behaviour change: How to abort the gets() when you don't have
readline?

Doing it with a flag is a lot more susceptable to subtle behaviour
changes, but I'll see if I can make it work.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

On Fri, Oct 21, 2005 at 01:53:32PM +0200, Martijn van Oosterhout wrote:

On Thu, Oct 20, 2005 at 08:11:14PM -0400, mark@mark.mielke.cc wrote:

I disagree that psql should make *any* assumptions about what SIGINT
means to the child process. Consider less again, and Control-C used
to abort a search. You are suggesting that Control-C should not only
abort the search, but should also cut off the input from less. Less
won't die. Less will just see a terminated input stream. What has been
gained from this? Is this intuitive behaviour?

I must say I agree with the idea that Ctrl-C shouldn't stop the stream
from psql, but I'm willing to let it slide because a lot of other
programs work this way. I imagine asking it to be configurable will
meet even more resistance.

Which other ones? I can't think of one. The ones that don't handle
SIGINT, or that are not designed for this scenario certainly don't count -
as that is how psql works right now without the patch. Of the remaining
programs that are designed to work with an application such as less, which
ones purposefully close the input stream to less, and continue running?

I think 0.

I think the only reasonable behaviour is to ignore SIGINT within the
parent, until the child exits. I don't see why other behaviours are
even being considered. To me, it points at a misunderstanding of the
problem.

I've been playing with a version of psql which on Ctrl-C doesn't
longjmp() but politely frees everything, waits for the pager and then
back to the main loop with the message "Interrupted". But now we have
another behaviour change: How to abort the gets() when you don't have
readline?
Doing it with a flag is a lot more susceptable to subtle behaviour
changes, but I'll see if I can make it work.

SIG_IGN is the easiest, and the most effective. Certainly the easiest
to maintain, and the least intrusive to the core.

I don't understand.

Cheers,
mark

--
mark@mielke.cc / markm@ncf.ca / markm@nortel.com __________________________
. . _ ._ . . .__ . . ._. .__ . . . .__ | Neighbourhood Coder
|\/| |_| |_| |/ |_ |\/| | |_ | |/ |_ |
| | | | | \ | \ |__ . | | .|. |__ |__ | \ |__ | Ottawa, Ontario, Canada

One ring to rule them all, one ring to find them, one ring to bring them all
and in the darkness bind them...

http://mark.mielke.cc/

On Fri, Oct 21, 2005 at 08:48:31AM -0400, mark@mark.mielke.cc wrote:

Which other ones? I can't think of one. The ones that don't handle
SIGINT, or that are not designed for this scenario certainly don't count -
as that is how psql works right now without the patch. Of the remaining
programs that are designed to work with an application such as less, which
ones purposefully close the input stream to less, and continue running?

The one run into all the time is:

rgrep "something" . | less

If you do a search and it doesn't find anything, it'll keep reading in
data until it finds it or reaches the end. Hit Ctrl-C to abort the
search and you kill the rgrep too.

Now, arguably rgrep is not "designed to work with an application such
as less" so doesn't meet your criteria. So the argument is about
whether psql is. Is the pager an integral part of the output mechanism
of psql, or just a bolt-on like a shell pipe? I suspect you can argue
either way, which is why I'm trying to find a way to have it both ways.

SIG_IGN is the easiest, and the most effective. Certainly the easiest
to maintain, and the least intrusive to the core.

Yeah, but doesn't fix the memory leaks and other random leakage and
breakage from the longjmp(). But I'm getting there...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

mark@mark.mielke.cc wrote:

On Thu, Oct 20, 2005 at 03:42:10PM -0700, Kevin Brown wrote:

Martijn van Oosterhout wrote:

You can't do a pclose in a signal handler, it's not one of the
"reeentrant safe" functions and could lead to deadlocks. The signal
manpage documents the ones you can use. Just set a flag. Setting the
descriptor to NULL is worse because then we have check before every
output function. fprintf(NULL, ...) will segfault on most
architechtures I wager.

Yeah, I was thinking that you'd do the check for the flag and invoke a
cleanup handler after the write() to the output file descriptor. It's
not clear that you'd need to do the check anyplace else. It's been a
while since I've messed with this stuff, but if I recall correctly,
the write() will return immediately after receipt of a signal, and
will indicate how much was actually written. So receipt of a SIGINT
should wind up being handled in a reasonably timely fashion.

Additionally the normal SIGINT signal handler (the one that gets
invoked when the pager is turned off) can be called from the cleanup
handler in order to maintain the proper semantics.

I disagree that psql should make *any* assumptions about what SIGINT
means to the child process. Consider less again, and Control-C used
to abort a search. You are suggesting that Control-C should not only
abort the search, but should also cut off the input from less. Less
won't die. Less will just see a terminated input stream. What has been
gained from this? Is this intuitive behaviour?

It's behaviour that's consistent with every other pipeline application
set I've ever seen.

The only difference between those and this situation is that for a
standard pipeline set, SIGINT will kill all the processes in the
pipeline. Less explicitly ignores (or traps) SIGINT, so the effect of
generating a SIGINT where less is at the end of the pipeline is to
kill everything that precedes it, and less will then show what results
it received. And obviously, that implies that the pipeline gets
closed.

If psql does not close the pipeline right then and there, then its
behaviour will obviously be different from what the user likely
expects, based on other pipelined uses of less. After all, if they
wanted to see the entire result set then they wouldn't have sent
SIGINT, would they?

If the pager does die in response to SIGINT, the write() will fail with
SIGPIPE. Completely clean, without any need for psql to pay attention
to SIGINT.

We're not talking about the semantics of the pager, we're talking
about the semantics of psql. You said it yourself: psql can't make
any assumptions about what SIGINT means to the child process. So it
has to consider what to do if the child does *not* die in response to
SIGINT. What are the proper semantics for psql + the child in that
situation?

Well, I think it's clear that having psql ignore SIGINT in that
situation is *not* correct, because it implies that whether or not
SIGINT causes processing to stop (as the user would expect) depends
entirely on the child. More specifically, it would depend on the
child either dying or explicitly closing its stdin upon receipt of
SIGINT.

The bottom line is that, at least in my opinion, the semantics of
SIGINT as regards psql should be the same whether or not there's a
pager involved, with one crucial difference: if there's a pager
involved, psql should wait for it to terminate before showing the
prompt. But otherwise, the semantics should be identical, because
they are what the user expects.

I think the only reasonable behaviour is to ignore SIGINT within the
parent, until the child exits. I don't see why other behaviours are
even being considered. To me, it points at a misunderstanding of the
problem.

Not at all. When you send SIGINT to a process, you want that process
to stop doing whatever it's doing and return control to you. That's
what it means, and that's what it's for. If we ignore SIGINT then
obviously we will *not* be heeding the wishes of the user who sends
SIGINT, and that is not likely what the user expects.

--
Kevin Brown kevin@sysexperts.com