From 8752cb283d65e40fc4096f84d99bcb93d97c59bc Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 13 Sep 2022 16:06:29 +0900 Subject: [PATCH] Simplify compression level handling in compression specifications --- src/include/common/compression.h | 3 +- src/backend/backup/basebackup_gzip.c | 10 +- src/backend/backup/basebackup_lz4.c | 9 +- src/backend/backup/basebackup_zstd.c | 11 +- src/common/compression.c | 105 +++++++++++++++---- src/bin/pg_basebackup/bbstreamer_gzip.c | 4 +- src/bin/pg_basebackup/bbstreamer_lz4.c | 3 +- src/bin/pg_basebackup/bbstreamer_zstd.c | 13 +-- src/bin/pg_basebackup/pg_basebackup.c | 19 +--- src/bin/pg_basebackup/pg_receivewal.c | 35 +------ src/bin/pg_basebackup/t/010_pg_basebackup.pl | 3 - 11 files changed, 107 insertions(+), 108 deletions(-) diff --git a/src/include/common/compression.h b/src/include/common/compression.h index 436b48104e..5d680058ed 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 { 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..409bf88101 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, + 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 (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/common/compression.c b/src/common/compression.c index da3c291c0f..ac26287d54 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 + * 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"), + get_compress_algorithm_name(spec->algorithm), + min_level, max_level); + /* * Of the compression algorithms that we currently support, only zstd * allows parallel workers. 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..32e1fb4b04 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, + /* 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)); - } + 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 9ce30d43a4..20d1baf675 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2003,27 +2003,12 @@ BaseBackup(char *compression_algorithm, char *compression_detail, */ if (includewal == STREAM_WAL) { - pg_compress_algorithm wal_compress_algorithm; - int wal_compress_level; - if (verbose) pg_log_info("starting background WAL receiver"); - 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; - } - else - { - wal_compress_algorithm = PG_COMPRESSION_NONE; - wal_compress_level = 0; - } - StartLogStreamer(xlogstart, starttli, sysidentifier, - wal_compress_algorithm, wal_compress_level); + client_compress->algorithm, + client_compress->level); } if (serverMajor >= 1500) diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c index f064cff4ab..a6e3387a6d 100644 --- a/src/bin/pg_basebackup/pg_receivewal.c +++ b/src/bin/pg_basebackup/pg_receivewal.c @@ -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. diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 3d1a4ddd5c..40f1d3f7e2 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -860,9 +860,6 @@ SKIP: my $gzip_is_valid = system_log($gzip, '--test', @zlib_files, @zlib_files2, @zlib_files3); is($gzip_is_valid, 0, "gzip verified the integrity of compressed data"); - rmtree("$tempdir/backup_gzip"); - rmtree("$tempdir/backup_gzip2"); - rmtree("$tempdir/backup_gzip3"); } # Test background stream process terminating before the basebackup has -- 2.37.2