Refactor tar method of walmethods.c to rely on the compression method

Since d62bcc8, the directory method of walmethods.c uses the compression
method to determine which code path to take.  The tar method, used by
pg_basebackup --format=t, was inconsistent regarding that, as it relied
on the compression level to check if no compression or gzip should be
used.  This commit makes the code more consistent as a whole in this
file, making the tar logic use a compression method rather than
assigning COMPRESSION_NONE that would be ignored.

The options of pg_basebackup are planned to be reworked but we are not
sure yet of the shape they should have as this has some dependency with
the integration of the server-side compression for base backups, so this
is left out for the moment.  This change has as benefit to make easier
the future integration of new compression methods for the tar method of
walmethods.c, for the client-side compression.

Reviewed-by: Georgios Kokolatos
Discussion: https://postgr.es/m/Yb3GEgWwcu4wZDuA@paquier.xyz

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1739ac638231c316521a53cf80e9c7855db7916d..17ff0132d9bcd7d1421cfcadf6a1dca1e1179aa5 100644 (file)
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -524,7 +524,8 @@ LogStreamerMain(logstreamer_param *param)
                                                    stream.do_sync);
    else
        stream.walmethod = CreateWalTarMethod(param->xlog,
-                                             COMPRESSION_NONE, /* ignored */
+                                             (compresslevel > 0) ?
+                                             COMPRESSION_GZIP : COMPRESSION_NONE,
                                              compresslevel,
                                              stream.do_sync);
 

diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index affdc5055fb6f9db97b457cf9dd1c8dac90534e3..424070531b87a30970f74b7e6f853567308c7f42 100644 (file)
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -749,7 +749,7 @@ tar_write(Walfile f, const void *buf, size_t count)
    tar_clear_error();
 
    /* Tarfile will always be positioned at the end */
-   if (!tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_NONE)
    {
        errno = 0;
        r = write(tar_data->fd, buf, count);
@@ -763,21 +763,20 @@ tar_write(Walfile f, const void *buf, size_t count)
        return r;
    }
 #ifdef HAVE_LIBZ
-   else
+   else if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        if (!tar_write_compressed_data(unconstify(void *, buf), count, false))
            return -1;
        ((TarMethodFile *) f)->currpos += count;
        return count;
    }
-#else
+#endif
    else
    {
-       /* Can't happen - compression enabled with no libz */
+       /* Can't happen - compression enabled with no method set */
        tar_data->lasterrno = ENOSYS;
        return -1;
    }
-#endif
 }
 
 static bool
@@ -833,7 +832,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        }
 
 #ifdef HAVE_LIBZ
-       if (tar_data->compression_level)
+       if (tar_data->compression_method == COMPRESSION_GZIP)
        {
            tar_data->zp = (z_streamp) pg_malloc(sizeof(z_stream));
            tar_data->zp->zalloc = Z_NULL;
@@ -884,7 +883,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
    pg_free(tmppath);
 
 #ifdef HAVE_LIBZ
-   if (tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        /* Flush existing data */
        if (!tar_write_compressed_data(NULL, 0, true))
@@ -909,7 +908,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
    }
    tar_data->currentfile->currpos = 0;
 
-   if (!tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_NONE)
    {
        errno = 0;
        if (write(tar_data->fd, tar_data->currentfile->header,
@@ -923,7 +922,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        }
    }
 #ifdef HAVE_LIBZ
-   else
+   else if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        /* Write header through the zlib APIs but with no compression */
        if (!tar_write_compressed_data(tar_data->currentfile->header,
@@ -938,6 +937,11 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
        }
    }
 #endif
+   else
+   {
+       /* not reachable */
+       Assert(false);
+   }
 
    tar_data->currentfile->pathname = pg_strdup(pathname);
 
@@ -948,7 +952,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
    if (pad_to_size)
    {
        tar_data->currentfile->pad_to_size = pad_to_size;
-       if (!tar_data->compression_level)
+       if (tar_data->compression_method == COMPRESSION_NONE)
        {
            /* Uncompressed, so pad now */
            if (!tar_write_padding_data(tar_data->currentfile, pad_to_size))
@@ -1009,7 +1013,7 @@ tar_sync(Walfile f)
     * Always sync the whole tarfile, because that's all we can do. This makes
     * no sense on compressed files, so just ignore those.
     */
-   if (tar_data->compression_level)
+   if (tar_data->compression_method != COMPRESSION_NONE)
        return 0;
 
    r = fsync(tar_data->fd);
@@ -1030,7 +1034,7 @@ tar_close(Walfile f, WalCloseMethod method)
 
    if (method == CLOSE_UNLINK)
    {
-       if (tar_data->compression_level)
+       if (tar_data->compression_method != COMPRESSION_NONE)
        {
            tar_set_error("unlink not supported with compression");
            return -1;
@@ -1061,7 +1065,7 @@ tar_close(Walfile f, WalCloseMethod method)
     */
    if (tf->pad_to_size)
    {
-       if (tar_data->compression_level)
+       if (tar_data->compression_method == COMPRESSION_GZIP)
        {
            /*
             * A compressed tarfile is padded on close since we cannot know
@@ -1102,7 +1106,7 @@ tar_close(Walfile f, WalCloseMethod method)
 
 
 #ifdef HAVE_LIBZ
-   if (tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        /* Flush the current buffer */
        if (!tar_write_compressed_data(NULL, 0, true))
@@ -1131,7 +1135,7 @@ tar_close(Walfile f, WalCloseMethod method)
        tar_data->lasterrno = errno;
        return -1;
    }
-   if (!tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_NONE)
    {
        errno = 0;
        if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
@@ -1142,7 +1146,7 @@ tar_close(Walfile f, WalCloseMethod method)
        }
    }
 #ifdef HAVE_LIBZ
-   else
+   else if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        /* Turn off compression */
        if (deflateParams(tar_data->zp, 0, 0) != Z_OK)
@@ -1164,6 +1168,11 @@ tar_close(Walfile f, WalCloseMethod method)
        }
    }
 #endif
+   else
+   {
+       /* not reachable */
+       Assert(false);
+   }
 
    /* Move file pointer back down to end, so we can write the next file */
    if (lseek(tar_data->fd, 0, SEEK_END) < 0)
@@ -1212,7 +1221,7 @@ tar_finish(void)
 
    /* A tarfile always ends with two empty blocks */
    MemSet(zerobuf, 0, sizeof(zerobuf));
-   if (!tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_NONE)
    {
        errno = 0;
        if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
@@ -1223,7 +1232,7 @@ tar_finish(void)
        }
    }
 #ifdef HAVE_LIBZ
-   else
+   else if (tar_data->compression_method == COMPRESSION_GZIP)
    {
        if (!tar_write_compressed_data(zerobuf, sizeof(zerobuf), false))
            return false;
@@ -1268,6 +1277,11 @@ tar_finish(void)
        }
    }
 #endif
+   else
+   {
+       /* not reachable */
+       Assert(false);
+   }
 
    /* sync the empty blocks as well, since they're after the last file */
    if (tar_data->sync)
@@ -1312,7 +1326,8 @@ CreateWalTarMethod(const char *tarbase,
                   int compression_level, bool sync)
 {
    WalWriteMethod *method;
-   const char *suffix = (compression_level != 0) ? ".tar.gz" : ".tar";
+   const char *suffix = (compression_method == COMPRESSION_GZIP) ?
+   ".tar.gz" : ".tar";
 
    method = pg_malloc0(sizeof(WalWriteMethod));
    method->open_for_write = tar_open_for_write;
@@ -1335,7 +1350,7 @@ CreateWalTarMethod(const char *tarbase,
    tar_data->compression_level = compression_level;
    tar_data->sync = sync;
 #ifdef HAVE_LIBZ
-   if (compression_level)
+   if (compression_method == COMPRESSION_GZIP)
        tar_data->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
 #endif
 
@@ -1347,7 +1362,7 @@ FreeWalTarMethod(void)
 {
    pg_free(tar_data->tarfilename);
 #ifdef HAVE_LIBZ
-   if (tar_data->compression_level)
+   if (tar_data->compression_method == COMPRESSION_GZIP)
        pg_free(tar_data->zlibOut);
 #endif
    pg_free(tar_data);