Horton

Thread

Commits

  1. Clean up error handling in pg_basebackup's walmethods.c.

  2. Handle close() failures more robustly in pg_dump and pg_basebackup.

  3. Add support for LZ4 compression in pg_receivewal

  1. Deficient error handling in pg_dump and pg_basebackup

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-09T20:20:31Z

    Coverity complained about this code in bbstreamer_file.c,
saying that the get_gz_error() call could be accessing freed
memory:

    if (gzclose(mystreamer->gzfile) != 0)
    {
        pg_log_error("could not close compressed file \"%s\": %s",
                     mystreamer->pathname,
                     get_gz_error(mystreamer->gzfile));
        exit(1);
    }

I think it's right.  This certainly isn't sanctioned by the zlib
specification [1], nor does it look like our other gzclose() calls,
which simply consult errno.  (Note that this coding is not at all new,
but Coverity thinks it is because 23a1c6578 moved it to a new file.)

I set out to fix that, intending only to replace the use of
get_gz_error() with %m, but the patch soon metastasized to the
point where I think it needs review :-(.

0001 below addresses places that either didn't check the result of
gzclose() at all, or neglected to print a useful error message.
The zlib spec doesn't quite promise that a failed gzclose() will
set errno, but the cases where it might not seem like can't-happen
cases that we needn't spend much effort on.  So what I've done here
is just to initialize errno to zero before gzclose().  If we ever
get a report of "could not close file: Success" in one of these
places, we'll know to look more closely.

0002 below addresses a rather messier problem, which is that the
error handling in src/bin/pg_basebackup/walmethods.c is just
appallingly bad.  The design is awful, because it relies on errno
to hold still for much longer than we can sanely expect --- notably,
walmethods.c really shouldn't be assuming much about what the callers
might do between calling an error-generating method and calling
getlasterror().  Worse, there are a lot of places that naively expect
errno to hold still across free() and similar operations.  We know
that not to be the case on (at least) macOS.  On top of that,
commit babbbb595 (LZ4 support) broke the design completely, because
liblz4 doesn't use errno to report errors.  Separately from the
question of how to report errors, there were a number of places
that failed to detect errors in the first place, and more that were
sloppy about setting the correct error code for a short write.

What I chose to do to fix the design problem is to use a modified
version of the idea that existed in the tar-methods part of the file,
which is to have both a string field and an errno field in the
DirectoryMethodData and TarMethodData objects.  If the string isn't
NULL then it overrides the errno field, allowing us to report
non-errno problems.  Then, a bunch of places have to remember to copy
errno into the lasterrno field, which is a bit annoying.  On the other
hand, we can get rid of logic that tries to save and restore errno
across functions that might change it, so this does buy back some
complexity too.

There's at least one loose end here, which is that there's one
place in tar_close() that does a summary exit(1) on fsync failure,
without even bothering with an error message.  I added the
missing error report:
 
     /* Always fsync on close, so the padding gets fsynced */
     if (tar_sync(f) < 0)
+    {
+        /* XXX this seems pretty bogus; why is only this case fatal? */
+        pg_log_fatal("could not fsync file \"%s\": %s",
+                     tf->pathname, tar_getlasterror());
         exit(1);
+    }

but this code seems about as fishy and ill-thought-out as anything
else I've touched here.  Why is this different from the half-dozen
other fsync-error cases in the same file?  Why, if fsync failure
here is so catastrophic, is it okay to just return a normal failure
code when tar_close doesn't even get to this point because of some
earlier error?  At the very least it seems like it'd be preferable
to do the exit(1) at the caller level.

The addition of the exit(1) seems to have been intentional in
1e2fddfa3, so cc'ing Peter for comment.

			regards, tom lane

[1] https://refspecs.linuxbase.org/LSB_3.0.0/LSB-Core-generic/LSB-Core-generic/zlib-gzclose-1.html

  2. Re: Deficient error handling in pg_dump and pg_basebackup

    Michael Paquier <michael@paquier.xyz> — 2021-11-10T05:03:11Z

    On Tue, Nov 09, 2021 at 03:20:31PM -0500, Tom Lane wrote:
> I think it's right.  This certainly isn't sanctioned by the zlib
> specification [1], nor does it look like our other gzclose() calls,
> which simply consult errno.  (Note that this coding is not at all new,
> but Coverity thinks it is because 23a1c6578 moved it to a new file.)
> 
> I set out to fix that, intending only to replace the use of
> get_gz_error() with %m, but the patch soon metastasized to the
> point where I think it needs review :-(.

This points gzclose_w() to gzwrite.c.  Looking at its end, it could
return Z_ERRNO after freeing everything when failing to close the
state's fd.  So I agree that coverity is right about this
possibility.  That's unlikely going to happen, though.

> 0001 below addresses places that either didn't check the result of
> gzclose() at all, or neglected to print a useful error message.
> The zlib spec doesn't quite promise that a failed gzclose() will
> set errno, but the cases where it might not seem like can't-happen
> cases that we needn't spend much effort on.

The zlib code does not do much regarding that, but system calls
would.  Still that joins with the point you are doing below where
free() may not hold errno, and note that the last thing gzclose() does
is a free().  :p

> So what I've done here
> is just to initialize errno to zero before gzclose().  If we ever
> get a report of "could not close file: Success" in one of these
> places, we'll know to look more closely.

Okay.

> On top of that,
> commit babbbb595 (LZ4 support) broke the design completely, because
> liblz4 doesn't use errno to report errors.  Separately from the
> question of how to report errors, there were a number of places
> that failed to detect errors in the first place, and more that were
> sloppy about setting the correct error code for a short write.

This one's my fault, sorry about that :/

What you are doing here looks fine for the LZ4F_isError() paths.

> What I chose to do to fix the design problem is to use a modified
> version of the idea that existed in the tar-methods part of the file,
> which is to have both a string field and an errno field in the
> DirectoryMethodData and TarMethodData objects.  If the string isn't
> NULL then it overrides the errno field, allowing us to report
> non-errno problems.  Then, a bunch of places have to remember to copy
> errno into the lasterrno field, which is a bit annoying.  On the other
> hand, we can get rid of logic that tries to save and restore errno
> across functions that might change it, so this does buy back some
> complexity too.

This makes the code a bit harder to follow when it comes to cascading
calls with tar_write_*() or tar_close(), but your approach looks
simple enough for this code.  I don't think you have missed a spot
after scanning the whole area.

> There's at least one loose end here, which is that there's one
> place in tar_close() that does a summary exit(1) on fsync failure,
> without even bothering with an error message.  I added the
> missing error report:
>  
>      /* Always fsync on close, so the padding gets fsynced */
>      if (tar_sync(f) < 0)
> +    {
> +        /* XXX this seems pretty bogus; why is only this case fatal? */
> +        pg_log_fatal("could not fsync file \"%s\": %s",
> +                     tf->pathname, tar_getlasterror());
>          exit(1);
> +    }

> but this code seems about as fishy and ill-thought-out as anything
> else I've touched here.  Why is this different from the half-dozen
> other fsync-error cases in the same file?  Why, if fsync failure
> here is so catastrophic, is it okay to just return a normal failure
> code when tar_close doesn't even get to this point because of some
> earlier error?

Hmm, I don't think that's fine.  There is an extra one in tar_finish()
that would report a failure, but not exit() immediately.  All the
callers of the sync callback in receivelog.c exit() on sight, but I am
wondering if it would not be saner to just exit() from walmethods.c
each time we see a failure with a fsync().

> At the very least it seems like it'd be preferable
> to do the exit(1) at the caller level.

You mean walmethods.c here, right?
--
Michael

  3. Re: Deficient error handling in pg_dump and pg_basebackup

    Michael Paquier <michael@paquier.xyz> — 2021-11-17T02:44:19Z

    On Wed, Nov 10, 2021 at 02:03:11PM +0900, Michael Paquier wrote:
>> but this code seems about as fishy and ill-thought-out as anything
>> else I've touched here.  Why is this different from the half-dozen
>> other fsync-error cases in the same file?  Why, if fsync failure
>> here is so catastrophic, is it okay to just return a normal failure
>> code when tar_close doesn't even get to this point because of some
>> earlier error?
> 
> Hmm, I don't think that's fine.  There is an extra one in tar_finish()
> that would report a failure, but not exit() immediately.  All the
> callers of the sync callback in receivelog.c exit() on sight, but I am
> wondering if it would not be saner to just exit() from walmethods.c
> each time we see a failure with a fsync().

Taking the issues with fsync() aside, which could be improved as a
separate patch, is there anything I can do for this thread?  The error
handling problems in walmethods.c introduced by the integration of LZ4
are on me, so I'd like to fix it.  0002 depends on some parts of 0001,
as well for walmethods.c and the new error code paths.  And I have
been through this code quite a lot recently.
--
Michael

  4. Re: Deficient error handling in pg_dump and pg_basebackup

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-17T03:26:11Z

    Michael Paquier <michael@paquier.xyz> writes:
> Taking the issues with fsync() aside, which could be improved as a
> separate patch, is there anything I can do for this thread?  The error
> handling problems in walmethods.c introduced by the integration of LZ4
> are on me, so I'd like to fix it.  0002 depends on some parts of 0001,
> as well for walmethods.c and the new error code paths.  And I have
> been through this code quite a lot recently.

I feel like doing an immediate exit() for these errors and not other
ones is a pretty terrible idea, mainly because it doesn't account for
the question of what to do with a failure that prevents us from getting
to the fsync() call in the first place.  So I'd like to see a better
design rather than more quick hacking.  I confess I don't have a clear
idea of what "a better design" would look like.

However, that's largely orthogonal to any of the things my proposed
patches are trying to fix.  If you want to review the patches without
considering the fsync-error-handling problem, that'd be great.

			regards, tom lane

  5. Re: Deficient error handling in pg_dump and pg_basebackup

    Michael Paquier <michael@paquier.xyz> — 2021-11-17T05:24:59Z

    On Tue, Nov 16, 2021 at 10:26:11PM -0500, Tom Lane wrote:
> I feel like doing an immediate exit() for these errors and not other
> ones is a pretty terrible idea, mainly because it doesn't account for
> the question of what to do with a failure that prevents us from getting
> to the fsync() call in the first place.  So I'd like to see a better
> design rather than more quick hacking.  I confess I don't have a
> clear idea of what "a better design" would look like.

[ .. thinks .. ]

We cannot really have something equivalent to data_sync_retry in the
frontends.  But I'd like to think that it would be fine for
pg_basebackup to just exit() on this failure so as callers would be
able to retry a base backup.  pg_receivewal is more tricky though.  An
exit() would allow for flush retries of a previous WAL segment where
things failed, but that stands when using --no-loop (still the code
path triggered by this option would not be used).  When not using
--no-loop, it may be better to actually just retry streaming from the
previous point so as the error should be reported from walmethods.c to
the upper stack anyway.

> However, that's largely orthogonal to any of the things my proposed
> patches are trying to fix.  If you want to review the patches without
> considering the fsync-error-handling problem, that'd be great.

I have looked at them upthread, FWIW:
https://www.postgresql.org/message-id/YYtSj5vlWp5faVXz@paquier.xyz
Your proposals still look rather sane to me, after a second look.
--
Michael

  6. Re: Deficient error handling in pg_dump and pg_basebackup

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-17T19:19:20Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Nov 16, 2021 at 10:26:11PM -0500, Tom Lane wrote:
>> However, that's largely orthogonal to any of the things my proposed
>> patches are trying to fix.  If you want to review the patches without
>> considering the fsync-error-handling problem, that'd be great.

> I have looked at them upthread, FWIW:
> https://www.postgresql.org/message-id/YYtSj5vlWp5faVXz@paquier.xyz
> Your proposals still look rather sane to me, after a second look.

Pushed then; thanks for reviewing that.  We can consider the fsync
error question at leisure.

			regards, tom lane

  7. Re: Deficient error handling in pg_dump and pg_basebackup

    Michael Paquier <michael@paquier.xyz> — 2021-11-18T00:29:32Z

    On Wed, Nov 17, 2021 at 02:19:20PM -0500, Tom Lane wrote:
> Pushed then; thanks for reviewing that.  We can consider the fsync
> error question at leisure.

Fine by me.  Thanks for the commit.
--
Michael

  8. Re: Deficient error handling in pg_dump and pg_basebackup

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-11-19T10:10:46Z

    On 09.11.21 21:20, Tom Lane wrote:
> Why is this different from the half-dozen
> other fsync-error cases in the same file?  Why, if fsync failure
> here is so catastrophic, is it okay to just return a normal failure
> code when tar_close doesn't even get to this point because of some
> earlier error?  At the very least it seems like it'd be preferable
> to do the exit(1) at the caller level.
> 
> The addition of the exit(1) seems to have been intentional in
> 1e2fddfa3, so cc'ing Peter for comment.

That commit addressed the behavior of fsync() failure in pg_receivewal 
and pg_recvlogical, which are long-running daemon processes, so this 
change was analogous to the server-side changes at the time.  I don't 
know what the behavior of fsync() failure in pg_basebackup should be, so 
calls that are only reachable from pg_basebackup are currently being 
handled differently.