From 53332eacaff6a391f79294193d0c39bff9d14b43 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 14 Sep 2022 12:17:03 +0900 Subject: [PATCH] Simplify handling of compression level with compression specifications 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 --- doc/src/sgml/protocol.sgml | 10 +- src/backend/backup/basebackup_gzip.c | 10 +- src/backend/backup/basebackup_lz4.c | 9 +- src/backend/backup/basebackup_zstd.c | 13 +- src/bin/pg_basebackup/bbstreamer_gzip.c | 4 +- src/bin/pg_basebackup/bbstreamer_lz4.c | 3 +- src/bin/pg_basebackup/bbstreamer_zstd.c | 15 +-- src/bin/pg_basebackup/pg_basebackup.c | 7 +- src/bin/pg_basebackup/pg_receivewal.c | 35 +---- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 134 ++++++++++--------- src/common/compression.c | 105 ++++++++++++--- src/include/common/compression.h | 3 +- 12 files changed, 189 insertions(+), 159 deletions(-) diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index c549c0858a..946d563491 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -2752,9 +2752,13 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;" The level keyword sets the compression level. For gzip the compression level should be an - integer between 1 and 9, for lz4 an integer - between 1 and 12, and for zstd an integer - between 1 and 22. + integer between 1 and 9 + (default Z_DEFAULT_COMPRESSION or + -1), for lz4 an integer + between 1 and 12 (default 0 for fast compression + mode), and for zstd an integer between + 1 and 22 (default + ZSTD_CLEVEL_DEFAULT or 3). diff --git a/src/backend/backup/basebackup_gzip.c b/src/backend/backup/basebackup_gzip.c index a965866ff2..1ec42791f1 100644 --- a/src/backend/backup/basebackup_gzip.c +++ b/src/backend/backup/basebackup_gzip.c @@ -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; diff --git a/src/backend/backup/basebackup_lz4.c b/src/backend/backup/basebackup_lz4.c index d919e3dec7..986272ab9a 100644 --- a/src/backend/backup/basebackup_lz4.c +++ b/src/backend/backup/basebackup_lz4.c @@ -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; diff --git a/src/backend/backup/basebackup_zstd.c b/src/backend/backup/basebackup_zstd.c index 865067f8dc..84256e3fa2 100644 --- a/src/backend/backup/basebackup_zstd.c +++ b/src/backend/backup/basebackup_zstd.c @@ -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) { diff --git a/src/bin/pg_basebackup/bbstreamer_gzip.c b/src/bin/pg_basebackup/bbstreamer_gzip.c index e7261910d8..c3455ffbdd 100644 --- a/src/bin/pg_basebackup/bbstreamer_gzip.c +++ b/src/bin/pg_basebackup/bbstreamer_gzip.c @@ -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)); diff --git a/src/bin/pg_basebackup/bbstreamer_lz4.c b/src/bin/pg_basebackup/bbstreamer_lz4.c index b9752354c9..ed2aa01305 100644 --- a/src/bin/pg_basebackup/bbstreamer_lz4.c +++ b/src/bin/pg_basebackup/bbstreamer_lz4.c @@ -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)) diff --git a/src/bin/pg_basebackup/bbstreamer_zstd.c b/src/bin/pg_basebackup/bbstreamer_zstd.c index 8a839145a6..1207dd771a 100644 --- a/src/bin/pg_basebackup/bbstreamer_zstd.c +++ b/src/bin/pg_basebackup/bbstreamer_zstd.c @@ -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) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index ae73b4bc92..8c3eba0a61 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2020,9 +2020,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 { @@ -2031,7 +2029,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) diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index ea3902c971..e6c975d4f4 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -877,38 +877,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. diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 3d1a4ddd5c..f03f43d668 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -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. diff --git a/src/common/compression.c b/src/common/compression.c index da3c291c0f..e40ce98ef3 100644 --- a/src/common/compression.c +++ b/src/common/compression.c @@ -27,6 +27,13 @@ #include "postgres_fe.h" #endif +#ifdef USE_ZSTD +#include +#endif +#ifdef HAVE_LIBZ +#include +#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. diff --git a/src/include/common/compression.h b/src/include/common/compression.h index 7aa4240a27..bb80e881af 100644 --- a/src/include/common/compression.h +++ b/src/include/common/compression.h @@ -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 {