Re: [PATCH] Add SIGCHLD catch to psql

Greetings,

Toying around with FETCH_COUNT today, I discovered that it didn't do
the #1 thing I really wanted to use it for- query large tables without
having to worry about LIMIT to see the first couple hundred records.
The reason is simple- psql ignores $PAGER exiting, which means that it
will happily continue pulling down the entire large table long after
you've stopped caring, which means you still have to wait forever.

The attached, admittedly quick hack, fixes this by having psql catch
SIGCHLD's using handle_sigint. I've tested this and it doesn't
appear to obviously break other cases where we have children (\!, for
example), since we're not going to be running a database query when
we're doing those, and if we are, and the child dies, we probably want
to *stop* anyway, similar to the $PAGER issue.

Another approach that I considered was fixing various things to deal
cleanly with write's failing to $PAGER (I presume the writes *were*
failing, since less was in a defunct state, but I didn't actually
test). This solution was simpler, faster to code and check, and alot
less invasive (or so it seemed to me at the time).

Anyway, this makes FETCH_COUNT alot more useful, and, in my view, the
current behaviour of completely ignoring $PAGER exiting is a bug.

Thanks,

Stephen

Stephen Frost wrote:
-- Start of PGP signed section.
> Greetings,
>
> Toying around with FETCH_COUNT today, I discovered that it didn't do
> the #1 thing I really wanted to use it for- query large tables without
> having to worry about LIMIT to see the first couple hundred records.
> The reason is simple- psql ignores $PAGER exiting, which means that it
> will happily continue pulling down the entire large table long after
> you've stopped caring, which means you still have to wait forever.
>
> The attached, admittedly quick hack, fixes this by having psql catch
> SIGCHLD's using handle_sigint. I've tested this and it doesn't
> appear to obviously break other cases where we have children (\!, for
> example), since we're not going to be running a database query when
> we're doing those, and if we are, and the child dies, we probably want
> to *stop* anyway, similar to the $PAGER issue.
>
> Another approach that I considered was fixing various things to deal
> cleanly with write's failing to $PAGER (I presume the writes *were*
> failing, since less was in a defunct state, but I didn't actually
> test). This solution was simpler, faster to code and check, and alot
> less invasive (or so it seemed to me at the time).
>
> Anyway, this makes FETCH_COUNT alot more useful, and, in my view, the
> current behaviour of completely ignoring $PAGER exiting is a bug.

Plesae add this to the next commit-fest:

https://commitfest.postgresql.org/action/commitfest_view/inprogress

Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

On Fri, May 14, 2010 at 04:24:43PM -0400, Bruce Momjian wrote:
> Stephen Frost wrote:
> -- Start of PGP signed section.
> > Greetings,
> >
> > Toying around with FETCH_COUNT today, I discovered that it didn't do
> > the #1 thing I really wanted to use it for- query large tables without
> > having to worry about LIMIT to see the first couple hundred records.
> > The reason is simple- psql ignores $PAGER exiting, which means that it
> > will happily continue pulling down the entire large table long after
> > you've stopped caring, which means you still have to wait forever.
> >
> > The attached, admittedly quick hack, fixes this by having psql catch
> > SIGCHLD's using handle_sigint. I've tested this and it doesn't
> > appear to obviously break other cases where we have children (\!, for
> > example), since we're not going to be running a database query when
> > we're doing those, and if we are, and the child dies, we probably want
> > to *stop* anyway, similar to the $PAGER issue.
> >
> > Another approach that I considered was fixing various things to deal
> > cleanly with write's failing to $PAGER (I presume the writes *were*
> > failing, since less was in a defunct state, but I didn't actually
> > test). This solution was simpler, faster to code and check, and alot
> > less invasive (or so it seemed to me at the time).
> >
> > Anyway, this makes FETCH_COUNT alot more useful, and, in my view, the
> > current behaviour of completely ignoring $PAGER exiting is a bug.
>
> Plesae add this to the next commit-fest:
>
> https://commitfest.postgresql.org/action/commitfest_view/inprogress
>
> Thanks.

Wouldn't this count as a bug fix?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sat, May 15, 2010 at 7:46 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> >   Anyway, this makes FETCH_COUNT alot more useful, and, in my view, the
>> >   current behaviour of completely ignoring $PAGER exiting is a bug.
>>
>> Plesae add this to the next commit-fest:
>>
>>       https://commitfest.postgresql.org/action/commitfest_view/inprogress
>>
>> Thanks.
>
> Wouldn't this count as a bug fix?

Possibly, but changes to signal handlers are pretty global and can
sometimes have surprising side effects. I'm all in favor of someone
reviewing the patch - any volunteers? One case to test might be
reading input from a file that contains \! escapes. More generally,
we need to consider every way that psql can get SIGCHLD and think
about whether this is the right behavior.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, May 15, 2010 at 7:46 PM, David Fetter <david(at)fetter(dot)org> wrote:
>> Wouldn't this count as a bug fix?

> Possibly, but changes to signal handlers are pretty global and can
> sometimes have surprising side effects. I'm all in favor of someone
> reviewing the patch - any volunteers? One case to test might be
> reading input from a file that contains \! escapes. More generally,
> we need to consider every way that psql can get SIGCHLD and think
> about whether this is the right behavior.

I think this will introduce far more bugs than it fixes. A saner
approach, which would also help for other corner cases such as
out-of-disk-space, would be to check for write failures on the output
file and abandon the query if any occur.

regards, tom lane

Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Sat, May 15, 2010 at 7:46 PM, David Fetter <david(at)fetter(dot)org> wrote:
> >> Wouldn't this count as a bug fix?
>
> > Possibly, but changes to signal handlers are pretty global and can
> > sometimes have surprising side effects. I'm all in favor of someone
> > reviewing the patch - any volunteers? One case to test might be
> > reading input from a file that contains \! escapes. More generally,
> > we need to consider every way that psql can get SIGCHLD and think
> > about whether this is the right behavior.
>
> I think this will introduce far more bugs than it fixes. A saner
> approach, which would also help for other corner cases such as
> out-of-disk-space, would be to check for write failures on the output
> file and abandon the query if any occur.

Is this a TODO?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> A saner
> approach, which would also help for other corner cases such as
> out-of-disk-space, would be to check for write failures on the output
> file and abandon the query if any occur.

I had considered this, but I'm not sure we really need to catch *every*
write failure. Perhaps just catching if the '\n' at the end of a row
fails to be written out would be sufficient? Then turning around and
setting cancel_query might be enough.. I'll write that up and test if
it works.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> A saner
>> approach, which would also help for other corner cases such as
>> out-of-disk-space, would be to check for write failures on the output
>> file and abandon the query if any occur.

> I had considered this, but I'm not sure we really need to catch *every*
> write failure. Perhaps just catching if the '\n' at the end of a row
> fails to be written out would be sufficient?

If you're combining this with the FETCH_COUNT logic then it seems like
it'd be sufficient to check ferror(fout) once per fetch chunk, and just
fall out of that loop then. I don't want psql issuing query cancels
on its own authority, either.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> If you're combining this with the FETCH_COUNT logic then it seems like
> it'd be sufficient to check ferror(fout) once per fetch chunk, and just
> fall out of that loop then. I don't want psql issuing query cancels
> on its own authority, either.

Attached is a patch that just checks the result from the existing
fflush() inside the FETCH_COUNT loop and drops out of that loop if we
get an error from it.

Thanks!

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> If you're combining this with the FETCH_COUNT logic then it seems like
>> it'd be sufficient to check ferror(fout) once per fetch chunk, and just
>> fall out of that loop then. I don't want psql issuing query cancels
>> on its own authority, either.

> Attached is a patch that just checks the result from the existing
> fflush() inside the FETCH_COUNT loop and drops out of that loop if we
> get an error from it.

I thought it might be about that simple once you went at it the right
way ;-). However, I'd suggest checking ferror(pset.queryFout) as well
as the fflush result. It's not clear to me whether fflush should be
counted on to report an error that actually occurred in a previous
fwrite. (It's also unclear why fflush isn't documented to set the stream
error indicator on failure, but it isn't.)

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Attached is a patch that just checks the result from the existing
> > fflush() inside the FETCH_COUNT loop and drops out of that loop if we
> > get an error from it.
>
> I thought it might be about that simple once you went at it the right
> way ;-). However, I'd suggest checking ferror(pset.queryFout) as well
> as the fflush result. It's not clear to me whether fflush should be
> counted on to report an error that actually occurred in a previous
> fwrite. (It's also unclear why fflush isn't documented to set the stream
> error indicator on failure, but it isn't.)

Sure, I can add the ferror() check. Patch attached.

My man page (Debian/Linux) has this to say about fflush():

DESCRIPTION
The function fflush() forces a write of all user-space buffered
data for the given output or update stream via the stream’s
underlying write function. The open status of the stream
is unaffected.

If the stream argument is NULL, fflush() flushes all open output
streams.

For a non-locking counterpart, see unlocked_stdio(3).

RETURN VALUE
Upon successful completion 0 is returned. Otherwise, EOF is
returned and the global variable errno is set to indicate the
error.

ERRORS
EBADF Stream is not an open stream, or is not open for writing.

The function fflush() may also fail and set errno for any of the
errors specified for the routine write(2).

CONFORMING TO
C89, C99.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I thought it might be about that simple once you went at it the right
>> way ;-). However, I'd suggest checking ferror(pset.queryFout) as well
>> as the fflush result.

> Sure, I can add the ferror() check. Patch attached.

This seemed pretty small and uncontroversial, so I went ahead and
committed it for 9.0. I rearranged the order of operations a bit to
make it seem more coherent, and also added an initial clearerr() just
to forestall problems if stdout had the error flag set for some reason.

regards, tom lane