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

The error handling here was a mess, as a result of a fundamentally
bad design (relying on errno to keep its value much longer than is
safe to assume) as well as a lot of just plain sloppiness, both as
to noticing errors at all and as to reporting the correct errno.
Moreover, the recent addition of LZ4 compression broke things
completely, because liblz4 doesn't use errno to report errors.

To improve matters, keep the error state in the DirectoryMethodData or
TarMethodData struct, and add a string field so we can handle cases
that don't set errno.  (The tar methods already had a version of this,
but it can be done more efficiently since all these cases use a
constant error string.)  Make the dir and tar methods handle errors
in basically identical ways, which they didn't before.

This requires copying errno into the state struct in a lot of places,
which is a bit tedious, but it has the virtue that we can get rid of
ad-hoc code to save and restore errno in a number of places ... not
to mention that it fixes other places that should've saved/restored
errno but neglected to.

In passing, fix some pointlessly static buffers to be ordinary
local variables.

There remains an issue about exactly how to handle errors from
fsync(), but that seems like material for its own patch.

While the LZ4 problems are new, all the rest of this is fixes for
old bugs, so backpatch to v10 where walmethods.c was introduced.

Patch by me; thanks to Michael Paquier for review.

Discussion: https://postgr.es/m/1343113.1636489231@sss.pgh.pa.us

diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index 5cc10c6eba035543c260ac91b340d576f4d61f08..fe6b034637aa6b40344e2e8515d4703b0eba3f95 100644 (file)
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -27,6 +27,7 @@
 
 #include "common/file_perm.h"
 #include "common/file_utils.h"
+#include "common/logging.h"
 #include "pgtar.h"
 #include "receivelog.h"
 #include "streamutil.h"
@@ -51,6 +52,8 @@ typedef struct DirectoryMethodData
    WalCompressionMethod compression_method;
    int         compression_level;
    bool        sync;
+   const char *lasterrstring;  /* if set, takes precedence over lasterrno */
+   int         lasterrno;
 } DirectoryMethodData;
 static DirectoryMethodData *dir_data = NULL;
 
@@ -74,11 +77,17 @@ typedef struct DirectoryMethodFile
 #endif
 } DirectoryMethodFile;
 
+#define dir_clear_error() \
+   (dir_data->lasterrstring = NULL, dir_data->lasterrno = 0)
+#define dir_set_error(msg) \
+   (dir_data->lasterrstring = _(msg))
+
 static const char *
 dir_getlasterror(void)
 {
-   /* Directory method always sets errno, so just use strerror */
-   return strerror(errno);
+   if (dir_data->lasterrstring)
+       return dir_data->lasterrstring;
+   return strerror(dir_data->lasterrno);
 }
 
 static char *
@@ -98,7 +107,7 @@ dir_get_file_name(const char *pathname, const char *temp_suffix)
 static Walfile
 dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
 {
-   static char tmppath[MAXPGPATH];
+   char        tmppath[MAXPGPATH];
    char       *filename;
    int         fd;
    DirectoryMethodFile *f;
@@ -111,6 +120,8 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
    void       *lz4buf = NULL;
 #endif
 
+   dir_clear_error();
+
    filename = dir_get_file_name(pathname, temp_suffix);
    snprintf(tmppath, sizeof(tmppath), "%s/%s",
             dir_data->basedir, filename);
@@ -124,7 +135,10 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
     */
    fd = open(tmppath, O_WRONLY | O_CREAT | PG_BINARY, pg_file_create_mode);
    if (fd < 0)
+   {
+       dir_data->lasterrno = errno;
        return NULL;
+   }
 
 #ifdef HAVE_LIBZ
    if (dir_data->compression_method == COMPRESSION_GZIP)
@@ -132,6 +146,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        gzfp = gzdopen(fd, "wb");
        if (gzfp == NULL)
        {
+           dir_data->lasterrno = errno;
            close(fd);
            return NULL;
        }
@@ -139,6 +154,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        if (gzsetparams(gzfp, dir_data->compression_level,
                        Z_DEFAULT_STRATEGY) != Z_OK)
        {
+           dir_data->lasterrno = errno;
            gzclose(gzfp);
            return NULL;
        }
@@ -153,6 +169,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        ctx_out = LZ4F_createCompressionContext(&ctx, LZ4F_VERSION);
        if (LZ4F_isError(ctx_out))
        {
+           dir_data->lasterrstring = LZ4F_getErrorName(ctx_out);
            close(fd);
            return NULL;
        }
@@ -164,6 +181,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        header_size = LZ4F_compressBegin(ctx, lz4buf, lz4bufsize, NULL);
        if (LZ4F_isError(header_size))
        {
+           dir_data->lasterrstring = LZ4F_getErrorName(header_size);
            (void) LZ4F_freeCompressionContext(ctx);
            pg_free(lz4buf);
            close(fd);
@@ -173,17 +191,12 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        errno = 0;
        if (write(fd, lz4buf, header_size) != header_size)
        {
-           int         save_errno = errno;
-
+           /* If write didn't set errno, assume problem is no disk space */
+           dir_data->lasterrno = errno ? errno : ENOSPC;
            (void) LZ4F_compressEnd(ctx, lz4buf, lz4bufsize, NULL);
            (void) LZ4F_freeCompressionContext(ctx);
            pg_free(lz4buf);
            close(fd);
-
-           /*
-            * If write didn't set errno, assume problem is no disk space.
-            */
-           errno = save_errno ? save_errno : ENOSPC;
            return NULL;
        }
    }
@@ -201,24 +214,17 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
            errno = 0;
            if (write(fd, zerobuf.data, XLOG_BLCKSZ) != XLOG_BLCKSZ)
            {
-               int         save_errno = errno;
-
+               /* If write didn't set errno, assume problem is no disk space */
+               dir_data->lasterrno = errno ? errno : ENOSPC;
                close(fd);
-
-               /*
-                * If write didn't set errno, assume problem is no disk space.
-                */
-               errno = save_errno ? save_errno : ENOSPC;
                return NULL;
            }
        }
 
        if (lseek(fd, 0, SEEK_SET) != 0)
        {
-           int         save_errno = errno;
-
+           dir_data->lasterrno = errno;
            close(fd);
-           errno = save_errno;
            return NULL;
        }
    }
@@ -234,6 +240,7 @@ dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        if (fsync_fname(tmppath, false) != 0 ||
            fsync_parent_path(tmppath) != 0)
        {
+           dir_data->lasterrno = errno;
 #ifdef HAVE_LIBZ
            if (dir_data->compression_method == COMPRESSION_GZIP)
                gzclose(gzfp);
@@ -285,10 +292,19 @@ dir_write(Walfile f, const void *buf, size_t count)
    DirectoryMethodFile *df = (DirectoryMethodFile *) f;
 
    Assert(f != NULL);
+   dir_clear_error();
 
 #ifdef HAVE_LIBZ
    if (dir_data->compression_method == COMPRESSION_GZIP)
+   {
+       errno = 0;
        r = (ssize_t) gzwrite(df->gzfp, buf, count);
+       if (r != count)
+       {
+           /* If write didn't set errno, assume problem is no disk space */
+           dir_data->lasterrno = errno ? errno : ENOSPC;
+       }
+   }
    else
 #endif
 #ifdef HAVE_LIBLZ4
@@ -315,10 +331,18 @@ dir_write(Walfile f, const void *buf, size_t count)
                                             NULL);
 
            if (LZ4F_isError(compressed))
+           {
+               dir_data->lasterrstring = LZ4F_getErrorName(compressed);
                return -1;
+           }
 
+           errno = 0;
            if (write(df->fd, df->lz4buf, compressed) != compressed)
+           {
+               /* If write didn't set errno, assume problem is no disk space */
+               dir_data->lasterrno = errno ? errno : ENOSPC;
                return -1;
+           }
 
            inbuf = ((char *) inbuf) + chunk;
        }
@@ -328,7 +352,15 @@ dir_write(Walfile f, const void *buf, size_t count)
    }
    else
 #endif
+   {
+       errno = 0;
        r = write(df->fd, buf, count);
+       if (r != count)
+       {
+           /* If write didn't set errno, assume problem is no disk space */
+           dir_data->lasterrno = errno ? errno : ENOSPC;
+       }
+   }
    if (r > 0)
        df->currpos += r;
    return r;
@@ -338,6 +370,7 @@ static off_t
 dir_get_current_pos(Walfile f)
 {
    Assert(f != NULL);
+   dir_clear_error();
 
    /* Use a cached value to prevent lots of reseeks */
    return ((DirectoryMethodFile *) f)->currpos;
@@ -348,10 +381,11 @@ dir_close(Walfile f, WalCloseMethod method)
 {
    int         r;
    DirectoryMethodFile *df = (DirectoryMethodFile *) f;
-   static char tmppath[MAXPGPATH];
-   static char tmppath2[MAXPGPATH];
+   char        tmppath[MAXPGPATH];
+   char        tmppath2[MAXPGPATH];
 
    Assert(f != NULL);
+   dir_clear_error();
 
 #ifdef HAVE_LIBZ
    if (dir_data->compression_method == COMPRESSION_GZIP)
@@ -371,10 +405,18 @@ dir_close(Walfile f, WalCloseMethod method)
                                      NULL);
 
        if (LZ4F_isError(compressed))
+       {
+           dir_data->lasterrstring = LZ4F_getErrorName(compressed);
            return -1;
+       }
 
+       errno = 0;
        if (write(df->fd, df->lz4buf, compressed) != compressed)
+       {
+           /* If write didn't set errno, assume problem is no disk space */
+           dir_data->lasterrno = errno ? errno : ENOSPC;
            return -1;
+       }
 
        r = close(df->fd);
    }
@@ -433,6 +475,9 @@ dir_close(Walfile f, WalCloseMethod method)
        }
    }
 
+   if (r != 0)
+       dir_data->lasterrno = errno;
+
 #ifdef HAVE_LIBLZ4
    pg_free(df->lz4buf);
    /* supports free on NULL */
@@ -451,7 +496,10 @@ dir_close(Walfile f, WalCloseMethod method)
 static int
 dir_sync(Walfile f)
 {
+   int         r;
+
    Assert(f != NULL);
+   dir_clear_error();
 
    if (!dir_data->sync)
        return 0;
@@ -460,7 +508,10 @@ dir_sync(Walfile f)
    if (dir_data->compression_method == COMPRESSION_GZIP)
    {
        if (gzflush(((DirectoryMethodFile *) f)->gzfp, Z_SYNC_FLUSH) != Z_OK)
+       {
+           dir_data->lasterrno = errno;
            return -1;
+       }
    }
 #endif
 #ifdef HAVE_LIBLZ4
@@ -472,27 +523,41 @@ dir_sync(Walfile f)
        /* Flush any internal buffers */
        compressed = LZ4F_flush(df->ctx, df->lz4buf, df->lz4bufsize, NULL);
        if (LZ4F_isError(compressed))
+       {
+           dir_data->lasterrstring = LZ4F_getErrorName(compressed);
            return -1;
+       }
 
+       errno = 0;
        if (write(df->fd, df->lz4buf, compressed) != compressed)
+       {
+           /* If write didn't set errno, assume problem is no disk space */
+           dir_data->lasterrno = errno ? errno : ENOSPC;
            return -1;
+       }
    }
 #endif
 
-   return fsync(((DirectoryMethodFile *) f)->fd);
+   r = fsync(((DirectoryMethodFile *) f)->fd);
+   if (r < 0)
+       dir_data->lasterrno = errno;
+   return r;
 }
 
 static ssize_t
 dir_get_file_size(const char *pathname)
 {
    struct stat statbuf;
-   static char tmppath[MAXPGPATH];
+   char        tmppath[MAXPGPATH];
 
    snprintf(tmppath, sizeof(tmppath), "%s/%s",
             dir_data->basedir, pathname);
 
    if (stat(tmppath, &statbuf) != 0)
+   {
+       dir_data->lasterrno = errno;
        return -1;
+   }
 
    return statbuf.st_size;
 }
@@ -506,9 +571,11 @@ dir_compression_method(void)
 static bool
 dir_existsfile(const char *pathname)
 {
-   static char tmppath[MAXPGPATH];
+   char        tmppath[MAXPGPATH];
    int         fd;
 
+   dir_clear_error();
+
    snprintf(tmppath, sizeof(tmppath), "%s/%s",
             dir_data->basedir, pathname);
 
@@ -522,6 +589,8 @@ dir_existsfile(const char *pathname)
 static bool
 dir_finish(void)
 {
+   dir_clear_error();
+
    if (dir_data->sync)
    {
        /*
@@ -529,7 +598,10 @@ dir_finish(void)
         * directory entry here as well.
         */
        if (fsync_fname(dir_data->basedir, true) != 0)
+       {
+           dir_data->lasterrno = errno;
            return false;
+       }
    }
    return true;
 }
@@ -569,6 +641,7 @@ FreeWalDirectoryMethod(void)
 {
    pg_free(dir_data->basedir);
    pg_free(dir_data);
+   dir_data = NULL;
 }
 
 
@@ -594,7 +667,8 @@ typedef struct TarMethodData
    int         compression_level;
    bool        sync;
    TarMethodFile *currentfile;
-   char        lasterror[1024];
+   const char *lasterrstring;  /* if set, takes precedence over lasterrno */
+   int         lasterrno;
 #ifdef HAVE_LIBZ
    z_streamp   zp;
    void       *zlibOut;
@@ -602,19 +676,17 @@ typedef struct TarMethodData
 } TarMethodData;
 static TarMethodData *tar_data = NULL;
 
-#define tar_clear_error() tar_data->lasterror[0] = '\0'
-#define tar_set_error(msg) strlcpy(tar_data->lasterror, _(msg), sizeof(tar_data->lasterror))
+#define tar_clear_error() \
+   (tar_data->lasterrstring = NULL, tar_data->lasterrno = 0)
+#define tar_set_error(msg) \
+   (tar_data->lasterrstring = _(msg))
 
 static const char *
 tar_getlasterror(void)
 {
-   /*
-    * If a custom error is set, return that one. Otherwise, assume errno is
-    * set and return that one.
-    */
-   if (tar_data->lasterror[0])
-       return tar_data->lasterror;
-   return strerror(errno);
+   if (tar_data->lasterrstring)
+       return tar_data->lasterrstring;
+   return strerror(tar_data->lasterrno);
 }
 
 #ifdef HAVE_LIBZ
@@ -642,11 +714,8 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
            errno = 0;
            if (write(tar_data->fd, tar_data->zlibOut, len) != len)
            {
-               /*
-                * If write didn't set errno, assume problem is no disk space.
-                */
-               if (errno == 0)
-                   errno = ENOSPC;
+               /* If write didn't set errno, assume problem is no disk space */
+               tar_data->lasterrno = errno ? errno : ENOSPC;
                return false;
            }
 
@@ -683,9 +752,15 @@ tar_write(Walfile f, const void *buf, size_t count)
    /* Tarfile will always be positioned at the end */
    if (!tar_data->compression_level)
    {
+       errno = 0;
        r = write(tar_data->fd, buf, count);
-       if (r > 0)
-           ((TarMethodFile *) f)->currpos += r;
+       if (r != count)
+       {
+           /* If write didn't set errno, assume problem is no disk space */
+           tar_data->lasterrno = errno ? errno : ENOSPC;
+           return -1;
+       }
+       ((TarMethodFile *) f)->currpos += r;
        return r;
    }
 #ifdef HAVE_LIBZ
@@ -698,8 +773,11 @@ tar_write(Walfile f, const void *buf, size_t count)
    }
 #else
    else
+   {
        /* Can't happen - compression enabled with no libz */
+       tar_data->lasterrno = ENOSYS;
        return -1;
+   }
 #endif
 }
 
@@ -737,7 +815,6 @@ tar_get_file_name(const char *pathname, const char *temp_suffix)
 static Walfile
 tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
 {
-   int         save_errno;
    char       *tmppath;
 
    tar_clear_error();
@@ -751,7 +828,10 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
                            O_WRONLY | O_CREAT | PG_BINARY,
                            pg_file_create_mode);
        if (tar_data->fd < 0)
+       {
+           tar_data->lasterrno = errno;
            return NULL;
+       }
 
 #ifdef HAVE_LIBZ
        if (tar_data->compression_level)
@@ -782,7 +862,6 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        /* There's no tar header itself, the file starts with regular files */
    }
 
-   Assert(tar_data->currentfile == NULL);
    if (tar_data->currentfile != NULL)
    {
        tar_set_error("implementation error: tar files can't have more than one open file");
@@ -824,10 +903,9 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
    tar_data->currentfile->ofs_start = lseek(tar_data->fd, 0, SEEK_CUR);
    if (tar_data->currentfile->ofs_start == -1)
    {
-       save_errno = errno;
+       tar_data->lasterrno = errno;
        pg_free(tar_data->currentfile);
        tar_data->currentfile = NULL;
-       errno = save_errno;
        return NULL;
    }
    tar_data->currentfile->currpos = 0;
@@ -838,11 +916,10 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        if (write(tar_data->fd, tar_data->currentfile->header,
                  TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
        {
-           save_errno = errno;
+           /* If write didn't set errno, assume problem is no disk space */
+           tar_data->lasterrno = errno ? errno : ENOSPC;
            pg_free(tar_data->currentfile);
            tar_data->currentfile = NULL;
-           /* if write didn't set errno, assume problem is no disk space */
-           errno = save_errno ? save_errno : ENOSPC;
            return NULL;
        }
    }
@@ -875,12 +952,16 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        if (!tar_data->compression_level)
        {
            /* Uncompressed, so pad now */
-           tar_write_padding_data(tar_data->currentfile, pad_to_size);
+           if (!tar_write_padding_data(tar_data->currentfile, pad_to_size))
+               return NULL;
            /* Seek back to start */
            if (lseek(tar_data->fd,
                      tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE,
                      SEEK_SET) != tar_data->currentfile->ofs_start + TAR_BLOCK_SIZE)
+           {
+               tar_data->lasterrno = errno;
                return NULL;
+           }
 
            tar_data->currentfile->currpos = 0;
        }
@@ -895,7 +976,7 @@ tar_get_file_size(const char *pathname)
    tar_clear_error();
 
    /* Currently not used, so not supported */
-   errno = ENOSYS;
+   tar_data->lasterrno = ENOSYS;
    return -1;
 }
 
@@ -917,6 +998,8 @@ tar_get_current_pos(Walfile f)
 static int
 tar_sync(Walfile f)
 {
+   int         r;
+
    Assert(f != NULL);
    tar_clear_error();
 
@@ -930,7 +1013,10 @@ tar_sync(Walfile f)
    if (tar_data->compression_level)
        return 0;
 
-   return fsync(tar_data->fd);
+   r = fsync(tar_data->fd);
+   if (r < 0)
+       tar_data->lasterrno = errno;
+   return r;
 }
 
 static int
@@ -957,7 +1043,10 @@ tar_close(Walfile f, WalCloseMethod method)
         * allow writing of the very last file.
         */
        if (ftruncate(tar_data->fd, tf->ofs_start) != 0)
+       {
+           tar_data->lasterrno = errno;
            return -1;
+       }
 
        pg_free(tf->pathname);
        pg_free(tf);
@@ -1018,10 +1107,7 @@ tar_close(Walfile f, WalCloseMethod method)
    {
        /* Flush the current buffer */
        if (!tar_write_compressed_data(NULL, 0, true))
-       {
-           errno = EINVAL;
            return -1;
-       }
    }
 #endif
 
@@ -1042,15 +1128,17 @@ tar_close(Walfile f, WalCloseMethod method)
 
    print_tar_number(&(tf->header[148]), 8, tarChecksum(((TarMethodFile *) f)->header));
    if (lseek(tar_data->fd, tf->ofs_start, SEEK_SET) != ((TarMethodFile *) f)->ofs_start)
+   {
+       tar_data->lasterrno = errno;
        return -1;
+   }
    if (!tar_data->compression_level)
    {
        errno = 0;
        if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
        {
-           /* if write didn't set errno, assume problem is no disk space */
-           if (errno == 0)
-               errno = ENOSPC;
+           /* If write didn't set errno, assume problem is no disk space */
+           tar_data->lasterrno = errno ? errno : ENOSPC;
            return -1;
        }
    }
@@ -1080,11 +1168,19 @@ tar_close(Walfile f, WalCloseMethod method)
 
    /* Move file pointer back down to end, so we can write the next file */
    if (lseek(tar_data->fd, 0, SEEK_END) < 0)
+   {
+       tar_data->lasterrno = errno;
        return -1;
+   }
 
    /* 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);
+   }
 
    /* Clean up and done */
    pg_free(tf->pathname);
@@ -1122,9 +1218,8 @@ tar_finish(void)
        errno = 0;
        if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
        {
-           /* if write didn't set errno, assume problem is no disk space */
-           if (errno == 0)
-               errno = ENOSPC;
+           /* If write didn't set errno, assume problem is no disk space */
+           tar_data->lasterrno = errno ? errno : ENOSPC;
            return false;
        }
    }
@@ -1159,8 +1254,7 @@ tar_finish(void)
                     * If write didn't set errno, assume problem is no disk
                     * space.
                     */
-                   if (errno == 0)
-                       errno = ENOSPC;
+                   tar_data->lasterrno = errno ? errno : ENOSPC;
                    return false;
                }
            }
@@ -1180,20 +1274,28 @@ tar_finish(void)
    if (tar_data->sync)
    {
        if (fsync(tar_data->fd) != 0)
+       {
+           tar_data->lasterrno = errno;
            return false;
+       }
    }
 
    if (close(tar_data->fd) != 0)
+   {
+       tar_data->lasterrno = errno;
        return false;
+   }
 
    tar_data->fd = -1;
 
    if (tar_data->sync)
    {
-       if (fsync_fname(tar_data->tarfilename, false) != 0)
-           return false;
-       if (fsync_parent_path(tar_data->tarfilename) != 0)
+       if (fsync_fname(tar_data->tarfilename, false) != 0 ||
+           fsync_parent_path(tar_data->tarfilename) != 0)
+       {
+           tar_data->lasterrno = errno;
            return false;
+       }
    }
 
    return true;
@@ -1250,4 +1352,5 @@ FreeWalTarMethod(void)
        pg_free(tar_data->zlibOut);
 #endif
    pg_free(tar_data);
+   tar_data = NULL;
 }