Simplify handling of compression level with compression specifications
authorMichael Paquier <[email protected]>
Wed, 14 Sep 2022 03:16:57 +0000 (12:16 +0900)
committerMichael Paquier <[email protected]>
Wed, 14 Sep 2022 03:16:57 +0000 (12:16 +0900)
PG_COMPRESSION_OPTION_LEVEL is removed from the compression
specification logic, and instead the compression level is always
assigned with each library's default if nothing is directly given.  This
centralizes the checks on the compression methods supported by a given
build, and always assigns a default compression level when parsing a
compression specification.  This results in complaining at an earlier
stage than previously if a build supports a compression method or not,
aka when parsing a specification in the backend or the frontend, and not
when processing it.  zstd, lz4 and zlib are able to handle in their
respective routines setting up the compression level the case of a
default value, hence the backend or frontend code (pg_receivewal or
pg_basebackup) has now no need to know what the default compression
level should be if nothing is specified: the logic is now done so as the
specification parsing assigns it.  It can also be enforced by passing
down a "level" set to the default value, that the backend will accept
(the replication protocol is for example able to handle a command like
BASE_BACKUP (COMPRESSION_DETAIL 'gzip:level=-1')).

This code simplification fixes an issue with pg_basebackup --gzip
introduced by ffd5365, where the tarball of the streamed WAL segments
would be created as of pg_wal.tar.gz with uncompressed contents, while
the intention is to compress the segments with gzip at a default level.
The origin of the confusion comes from the handling of the default
compression level of gzip (-1 or Z_DEFAULT_COMPRESSION) and the value of
0 was getting assigned, which is what walmethods.c would consider
as equivalent to no compression when streaming WAL segments with its tar
methods.  Assigning always the compression level removes the confusion
of some code paths considering a value of 0 set in a specification as
either no compression or a default compression level.

Note that 010_pg_basebackup.pl has to be adjusted to skip a few tests
where the shape of the compression detail string for client and
server-side compression was checked using gzip.  This is a result of the
code simplification, as gzip specifications cannot be used if a build
does not support it.

Reported-by: Tom Lane
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/1400032.1662217889@sss.pgh.pa.us
Backpatch-through: 15

12 files changed:
doc/src/sgml/protocol.sgml
src/backend/backup/basebackup_gzip.c
src/backend/backup/basebackup_lz4.c
src/backend/backup/basebackup_zstd.c
src/bin/pg_basebackup/bbstreamer_gzip.c
src/bin/pg_basebackup/bbstreamer_lz4.c
src/bin/pg_basebackup/bbstreamer_zstd.c
src/bin/pg_basebackup/pg_basebackup.c
src/bin/pg_basebackup/pg_receivewal.c
src/bin/pg_basebackup/t/010_pg_basebackup.pl
src/common/compression.c
src/include/common/compression.h

index 87870c5b102eb3b45d81634dc68e20d69322f54c..dc08c9667d98abbed7a39bb8d1e2aa4c9aeca839 100644 (file)
@@ -2752,9 +2752,13 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
           <para>
            The <literal>level</literal> keyword sets the compression level.
            For <literal>gzip</literal> the compression level should be an
-           integer between 1 and 9, for <literal>lz4</literal> an integer
-           between 1 and 12, and for <literal>zstd</literal> an integer
-           between 1 and 22.
+           integer between <literal>1</literal> and <literal>9</literal>
+           (default <literal>Z_DEFAULT_COMPRESSION</literal> or
+           <literal>-1</literal>), for <literal>lz4</literal> an integer
+           between 1 and 12 (default <literal>0</literal> for fast compression
+           mode), and for <literal>zstd</literal> an integer between
+           <literal>1</literal> and <literal>22</literal> (default
+           <literal>ZSTD_CLEVEL_DEFAULT</literal> or <literal>3</literal>).
           </para>
 
           <para>
index a965866ff2bc82ff9bd16491799eb368229b36f6..1ec42791f1ea90ec7964f70ffbf58f2cfef220c5 100644 (file)
@@ -72,13 +72,9 @@ bbsink_gzip_new(bbsink *next, pg_compress_specification *compress)
 
        Assert(next != NULL);
 
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
-               compresslevel = Z_DEFAULT_COMPRESSION;
-       else
-       {
-               compresslevel = compress->level;
-               Assert(compresslevel >= 1 && compresslevel <= 9);
-       }
+       compresslevel = compress->level;
+       Assert((compresslevel >= 1 && compresslevel <= 9) ||
+                  compresslevel == Z_DEFAULT_COMPRESSION);
 
        sink = palloc0(sizeof(bbsink_gzip));
        *((const bbsink_ops **) &sink->base.bbs_ops) = &bbsink_gzip_ops;
index d919e3dec7ad4c9f1a7c08f57beef7f19864797f..986272ab9a5034d0516c60fb42ac1d3b68c27686 100644 (file)
@@ -72,13 +72,8 @@ bbsink_lz4_new(bbsink *next, pg_compress_specification *compress)
 
        Assert(next != NULL);
 
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
-               compresslevel = 0;
-       else
-       {
-               compresslevel = compress->level;
-               Assert(compresslevel >= 1 && compresslevel <= 12);
-       }
+       compresslevel = compress->level;
+       Assert(compresslevel >= 0 && compresslevel <= 12);
 
        sink = palloc0(sizeof(bbsink_lz4));
        *((const bbsink_ops **) &sink->base.bbs_ops) = &bbsink_lz4_ops;
index 865067f8dc990228f331e32eedd90ad60fc5502c..84256e3fa27b4e31015550f5255ce63278e07ad5 100644 (file)
@@ -96,14 +96,11 @@ bbsink_zstd_begin_backup(bbsink *sink)
        if (!mysink->cctx)
                elog(ERROR, "could not create zstd compression context");
 
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
-       {
-               ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
-                                                                        compress->level);
-               if (ZSTD_isError(ret))
-                       elog(ERROR, "could not set zstd compression level to %d: %s",
-                                compress->level, ZSTD_getErrorName(ret));
-       }
+       ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
+                                                                compress->level);
+       if (ZSTD_isError(ret))
+               elog(ERROR, "could not set zstd compression level to %d: %s",
+                        compress->level, ZSTD_getErrorName(ret));
 
        if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)
        {
index e7261910d8109131ff8af580a5f322a41e93b765..c3455ffbddf88f8195bc69ecf3edfd7ed5079ecd 100644 (file)
@@ -107,9 +107,7 @@ bbstreamer_gzip_writer_new(char *pathname, FILE *file,
                        pg_fatal("could not open output file: %m");
        }
 
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0 &&
-               gzsetparams(streamer->gzfile, compress->level,
-                                       Z_DEFAULT_STRATEGY) != Z_OK)
+       if (gzsetparams(streamer->gzfile, compress->level, Z_DEFAULT_STRATEGY) != Z_OK)
                pg_fatal("could not set compression level %d: %s",
                                 compress->level, get_gz_error(streamer->gzfile));
 
index b9752354c910325f606e7ae93e72a8f603127ffa..ed2aa01305719e9ad9c28a9df00af9f0916108ef 100644 (file)
@@ -88,8 +88,7 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, pg_compress_specification *compr
        prefs = &streamer->prefs;
        memset(prefs, 0, sizeof(LZ4F_preferences_t));
        prefs->frameInfo.blockSizeID = LZ4F_max256KB;
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
-               prefs->compressionLevel = compress->level;
+       prefs->compressionLevel = compress->level;
 
        ctxError = LZ4F_createCompressionContext(&streamer->cctx, LZ4F_VERSION);
        if (LZ4F_isError(ctxError))
index 8a839145a6523bec2b6315c915697566fbcf262e..1207dd771ae55bc0a2b949d520029c4bd7cf67d4 100644 (file)
@@ -84,15 +84,12 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, pg_compress_specification *comp
        if (!streamer->cctx)
                pg_fatal("could not create zstd compression context");
 
-       /* Set compression level, if specified */
-       if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
-       {
-               ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
-                                                                        compress->level);
-               if (ZSTD_isError(ret))
-                       pg_fatal("could not set zstd compression level to %d: %s",
-                                        compress->level, ZSTD_getErrorName(ret));
-       }
+       /* Set compression level */
+       ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
+                                                                compress->level);
+       if (ZSTD_isError(ret))
+               pg_fatal("could not set zstd compression level to %d: %s",
+                                compress->level, ZSTD_getErrorName(ret));
 
        /* Set # of workers, if specified */
        if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)
index 9ce30d43a417254728e7ed3d9507f4f0c16cf967..a830b199f54d271da2902a9de702ec6dfca08ac3 100644 (file)
@@ -2012,9 +2012,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
                if (client_compress->algorithm == PG_COMPRESSION_GZIP)
                {
                        wal_compress_algorithm = PG_COMPRESSION_GZIP;
-                       wal_compress_level =
-                               (client_compress->options & PG_COMPRESSION_OPTION_LEVEL)
-                               != 0 ? client_compress->level : 0;
+                       wal_compress_level = client_compress->level;
                }
                else
                {
@@ -2023,7 +2021,8 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
                }
 
                StartLogStreamer(xlogstart, starttli, sysidentifier,
-                                                wal_compress_algorithm, wal_compress_level);
+                                                wal_compress_algorithm,
+                                                wal_compress_level);
        }
 
        if (serverMajor >= 1500)
index f064cff4aba04df1199878ab16ccc536edcc22e0..a6e3387a6d48dc90d342efcdbd7fb6970dffc26e 100644 (file)
@@ -875,38 +875,11 @@ main(int argc, char **argv)
                pg_fatal("invalid compression specification: %s",
                                 error_detail);
 
-       /* Extract the compression level, if found in the specification */
-       if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) != 0)
-               compresslevel = compression_spec.level;
-
-       switch (compression_algorithm)
-       {
-               case PG_COMPRESSION_NONE:
-                       /* nothing to do */
-                       break;
-               case PG_COMPRESSION_GZIP:
-#ifdef HAVE_LIBZ
-                       if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) == 0)
-                       {
-                               pg_log_info("no value specified for --compress, switching to default");
-                               compresslevel = Z_DEFAULT_COMPRESSION;
-                       }
-#else
-                       pg_fatal("this build does not support compression with %s",
-                                        "gzip");
-#endif
-                       break;
-               case PG_COMPRESSION_LZ4:
-#ifndef USE_LZ4
-                       pg_fatal("this build does not support compression with %s",
-                                        "LZ4");
-#endif
-                       break;
-               case PG_COMPRESSION_ZSTD:
-                       pg_fatal("compression with %s is not yet supported", "ZSTD");
-                       break;
-       }
+       /* Extract the compression level */
+       compresslevel = compression_spec.level;
 
+       if (compression_algorithm == PG_COMPRESSION_ZSTD)
+               pg_fatal("compression with %s is not yet supported", "ZSTD");
 
        /*
         * Check existence of destination folder.
index 3d1a4ddd5c0ecb153af94221268741eaa9721b09..f03f43d66837fd2aaa41f30278d0e420a6628901 100644 (file)
@@ -86,71 +86,81 @@ $node->restart;
 # Now that we have a server that supports replication commands, test whether
 # certain invalid compression commands fail on the client side with client-side
 # compression and on the server side with server-side compression.
-my $client_fails = 'pg_basebackup: error: ';
-my $server_fails =
-  'pg_basebackup: error: could not initiate base backup: ERROR:  ';
-my @compression_failure_tests = (
-       [
-               'extrasquishy',
-               'unrecognized compression algorithm "extrasquishy"',
-               'failure on invalid compression algorithm'
-       ],
-       [
-               'gzip:',
-               'invalid compression specification: found empty string where a compression option was expected',
-               'failure on empty compression options list'
-       ],
-       [
-               'gzip:thunk',
-               'invalid compression specification: unknown compression option "thunk"',
-               'failure on unknown compression option'
-       ],
-       [
-               'gzip:level',
-               'invalid compression specification: compression option "level" requires a value',
-               'failure on missing compression level'
-       ],
-       [
-               'gzip:level=',
-               'invalid compression specification: value for compression option "level" must be an integer',
-               'failure on empty compression level'
-       ],
-       [
-               'gzip:level=high',
-               'invalid compression specification: value for compression option "level" must be an integer',
-               'failure on non-numeric compression level'
-       ],
-       [
-               'gzip:level=236',
-               'invalid compression specification: compression algorithm "gzip" expects a compression level between 1 and 9',
-               'failure on out-of-range compression level'
-       ],
-       [
-               'gzip:level=9,',
-               'invalid compression specification: found empty string where a compression option was expected',
-               'failure on extra, empty compression option'
-       ],
-       [
-               'gzip:workers=3',
-               'invalid compression specification: compression algorithm "gzip" does not accept a worker count',
-               'failure on worker count for gzip'
-       ],);
-for my $cft (@compression_failure_tests)
+SKIP:
 {
-       my $cfail = quotemeta($client_fails . $cft->[1]);
-       my $sfail = quotemeta($server_fails . $cft->[1]);
-       $node->command_fails_like(
-               [ 'pg_basebackup', '-D', "$tempdir/backup", '--compress', $cft->[0] ],
-               qr/$cfail/,
-               'client ' . $cft->[2]);
-       $node->command_fails_like(
+       skip "postgres was not built with ZLIB support", 6
+         if (!check_pg_config("#define HAVE_LIBZ 1"));
+
+       my $client_fails = 'pg_basebackup: error: ';
+       my $server_fails =
+         'pg_basebackup: error: could not initiate base backup: ERROR:  ';
+       my @compression_failure_tests = (
                [
-                       'pg_basebackup',   '-D',
-                       "$tempdir/backup", '--compress',
-                       'server-' . $cft->[0]
+                       'extrasquishy',
+                       'unrecognized compression algorithm "extrasquishy"',
+                       'failure on invalid compression algorithm'
                ],
-               qr/$sfail/,
-               'server ' . $cft->[2]);
+               [
+                       'gzip:',
+                       'invalid compression specification: found empty string where a compression option was expected',
+                       'failure on empty compression options list'
+               ],
+               [
+                       'gzip:thunk',
+                       'invalid compression specification: unknown compression option "thunk"',
+                       'failure on unknown compression option'
+               ],
+               [
+                       'gzip:level',
+                       'invalid compression specification: compression option "level" requires a value',
+                       'failure on missing compression level'
+               ],
+               [
+                       'gzip:level=',
+                       'invalid compression specification: value for compression option "level" must be an integer',
+                       'failure on empty compression level'
+               ],
+               [
+                       'gzip:level=high',
+                       'invalid compression specification: value for compression option "level" must be an integer',
+                       'failure on non-numeric compression level'
+               ],
+               [
+                       'gzip:level=236',
+                       'invalid compression specification: compression algorithm "gzip" expects a compression level between 1 and 9',
+                       'failure on out-of-range compression level'
+               ],
+               [
+                       'gzip:level=9,',
+                       'invalid compression specification: found empty string where a compression option was expected',
+                       'failure on extra, empty compression option'
+               ],
+               [
+                       'gzip:workers=3',
+                       'invalid compression specification: compression algorithm "gzip" does not accept a worker count',
+                       'failure on worker count for gzip'
+               ],);
+       for my $cft (@compression_failure_tests)
+       {
+               my $cfail = quotemeta($client_fails . $cft->[1]);
+               my $sfail = quotemeta($server_fails . $cft->[1]);
+               $node->command_fails_like(
+                       [
+                               'pg_basebackup',   '-D',
+                               "$tempdir/backup", '--compress',
+                               $cft->[0]
+                       ],
+                       qr/$cfail/,
+                       'client ' . $cft->[2]);
+               $node->command_fails_like(
+                       [
+                               'pg_basebackup',   '-D',
+                               "$tempdir/backup", '--compress',
+                               'server-' . $cft->[0]
+                       ],
+                       qr/$sfail/,
+                       'server ' . $cft->[2]);
+       }
 }
 
 # Write some files to test that they are not copied.
index da3c291c0ff39f73966aee3d353a7d75546c4e13..e40ce98ef3a80a1392b422ef6406895b982245d8 100644 (file)
 #include "postgres_fe.h"
 #endif
 
+#ifdef USE_ZSTD
+#include <zstd.h>
+#endif
+#ifdef HAVE_LIBZ
+#include <zlib.h>
+#endif
+
 #include "common/compression.h"
 
 static int     expect_integer_value(char *keyword, char *value,
@@ -88,6 +95,9 @@ get_compress_algorithm_name(pg_compress_algorithm algorithm)
  * Note, however, even if there's no parse error, the string might not make
  * sense: e.g. for gzip, level=12 is not sensible, but it does parse OK.
  *
+ * The compression level is assigned by default if not directly specified
+ * by the specification.
+ *
  * Use validate_compress_specification() to find out whether a compression
  * specification is semantically sensible.
  */
@@ -101,9 +111,46 @@ parse_compress_specification(pg_compress_algorithm algorithm, char *specificatio
        /* Initial setup of result object. */
        result->algorithm = algorithm;
        result->options = 0;
-       result->level = -1;
        result->parse_error = NULL;
 
+       /*
+        * Assign a default level depending on the compression method.  This may
+        * be enforced later.
+        */
+       switch (result->algorithm)
+       {
+               case PG_COMPRESSION_NONE:
+                       result->level = 0;
+                       break;
+               case PG_COMPRESSION_LZ4:
+#ifdef USE_LZ4
+                       result->level = 0;      /* fast compression mode */
+#else
+                       result->parse_error =
+                               psprintf(_("this build does not support compression with %s"),
+                                                "LZ4");
+#endif
+                       break;
+               case PG_COMPRESSION_ZSTD:
+#ifdef USE_ZSTD
+                       result->level = ZSTD_CLEVEL_DEFAULT;
+#else
+                       result->parse_error =
+                               psprintf(_("this build does not support compression with %s"),
+                                                "ZSTD");
+#endif
+                       break;
+               case PG_COMPRESSION_GZIP:
+#ifdef HAVE_LIBZ
+                       result->level = Z_DEFAULT_COMPRESSION;
+#else
+                       result->parse_error =
+                               psprintf(_("this build does not support compression with %s"),
+                                                "gzip");
+#endif
+                       break;
+       }
+
        /* If there is no specification, we're done already. */
        if (specification == NULL)
                return;
@@ -113,7 +160,6 @@ parse_compress_specification(pg_compress_algorithm algorithm, char *specificatio
        if (specification != bare_level_endp && *bare_level_endp == '\0')
        {
                result->level = bare_level;
-               result->options |= PG_COMPRESSION_OPTION_LEVEL;
                return;
        }
 
@@ -175,7 +221,11 @@ parse_compress_specification(pg_compress_algorithm algorithm, char *specificatio
                if (strcmp(keyword, "level") == 0)
                {
                        result->level = expect_integer_value(keyword, value, result);
-                       result->options |= PG_COMPRESSION_OPTION_LEVEL;
+
+                       /*
+                        * No need to set a flag in "options", there is a default level
+                        * set at least thanks to the logic above.
+                        */
                }
                else if (strcmp(keyword, "workers") == 0)
                {
@@ -249,36 +299,49 @@ expect_integer_value(char *keyword, char *value, pg_compress_specification *resu
 char *
 validate_compress_specification(pg_compress_specification *spec)
 {
+       int                     min_level = 1;
+       int                     max_level = 1;
+       int                     default_level = 0;
+
        /* If it didn't even parse OK, it's definitely no good. */
        if (spec->parse_error != NULL)
                return spec->parse_error;
 
        /*
-        * If a compression level was specified, check that the algorithm expects
-        * a compression level and that the level is within the legal range for
-        * the algorithm.
+        * Check that the algorithm expects a compression level and it is within
+        * the legal range for the algorithm.
         */
-       if ((spec->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
+       switch (spec->algorithm)
        {
-               int                     min_level = 1;
-               int                     max_level;
-
-               if (spec->algorithm == PG_COMPRESSION_GZIP)
+               case PG_COMPRESSION_GZIP:
                        max_level = 9;
-               else if (spec->algorithm == PG_COMPRESSION_LZ4)
+#ifdef HAVE_LIBZ
+                       default_level = Z_DEFAULT_COMPRESSION;
+#endif
+                       break;
+               case PG_COMPRESSION_LZ4:
                        max_level = 12;
-               else if (spec->algorithm == PG_COMPRESSION_ZSTD)
+                       default_level = 0;      /* fast mode */
+                       break;
+               case PG_COMPRESSION_ZSTD:
                        max_level = 22;
-               else
-                       return psprintf(_("compression algorithm \"%s\" does not accept a compression level"),
-                                                       get_compress_algorithm_name(spec->algorithm));
-
-               if (spec->level < min_level || spec->level > max_level)
-                       return psprintf(_("compression algorithm \"%s\" expects a compression level between %d and %d"),
-                                                       get_compress_algorithm_name(spec->algorithm),
-                                                       min_level, max_level);
+#ifdef USE_ZSTD
+                       default_level = ZSTD_CLEVEL_DEFAULT;
+#endif
+                       break;
+               case PG_COMPRESSION_NONE:
+                       if (spec->level != 0)
+                               return psprintf(_("compression algorithm \"%s\" does not accept a compression level"),
+                                                               get_compress_algorithm_name(spec->algorithm));
+                       break;
        }
 
+       if ((spec->level < min_level || spec->level > max_level) &&
+               spec->level != default_level)
+               return psprintf(_("compression algorithm \"%s\" expects a compression level between %d and %d (default at %d)"),
+                                               get_compress_algorithm_name(spec->algorithm),
+                                               min_level, max_level, default_level);
+
        /*
         * Of the compression algorithms that we currently support, only zstd
         * allows parallel workers.
index 436b48104e4aec8f90a88d2a467c899eb9c6ff7e..5d680058ed76072d9fe072b210b20df00f342cb8 100644 (file)
@@ -22,8 +22,7 @@ typedef enum pg_compress_algorithm
        PG_COMPRESSION_ZSTD
 } pg_compress_algorithm;
 
-#define PG_COMPRESSION_OPTION_LEVEL                    (1 << 0)
-#define PG_COMPRESSION_OPTION_WORKERS          (1 << 1)
+#define PG_COMPRESSION_OPTION_WORKERS          (1 << 0)
 
 typedef struct pg_compress_specification
 {