Horton
Menu

Thread

  1. [PATCH] Add SIGCHLD catch to psql

    Stephen Frost <sfrost@snowman.net> — 2010-05-14T15:41:38Z

    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

  2. Re: [PATCH] Add SIGCHLD catch to psql

    Bruce Momjian <bruce@momjian.us> — 2010-05-14T20:24:43Z

    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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  3. Re: [PATCH] Add SIGCHLD catch to psql

    David Fetter <david@fetter.org> — 2010-05-15T23:46:07Z

    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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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

  4. Re: [PATCH] Add SIGCHLD catch to psql

    Robert Haas <robertmhaas@gmail.com> — 2010-05-15T23:58:49Z

    On Sat, May 15, 2010 at 7:46 PM, David Fetter <david@fetter.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

  5. Re: [PATCH] Add SIGCHLD catch to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-16T00:09:23Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, May 15, 2010 at 7:46 PM, David Fetter <david@fetter.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

  6. Re: [PATCH] Add SIGCHLD catch to psql

    Bruce Momjian <bruce@momjian.us> — 2010-05-16T01:22:10Z

    Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Sat, May 15, 2010 at 7:46 PM, David Fetter <david@fetter.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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  7. Re: [PATCH] Add SIGCHLD catch to psql

    Stephen Frost <sfrost@snowman.net> — 2010-05-16T16:04:15Z

    * Tom Lane (tgl@sss.pgh.pa.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

  8. Re: [PATCH] Add SIGCHLD catch to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-16T16:22:33Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.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

  9. Re: [PATCH] Add SIGCHLD catch to psql

    Stephen Frost <sfrost@snowman.net> — 2010-05-17T16:15:10Z

    * Tom Lane (tgl@sss.pgh.pa.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

  10. Re: [PATCH] Add SIGCHLD catch to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-17T16:24:37Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.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

  11. Re: [PATCH] Add SIGCHLD catch to psql

    Stephen Frost <sfrost@snowman.net> — 2010-05-17T16:49:27Z

    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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

  12. Re: [PATCH] Add SIGCHLD catch to psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-28T20:04:18Z

    Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.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