On Sun, Sep 04, 2022 at 02:20:52PM +0900, Michael Paquier wrote: > ffd5365 has missed that wal_compress_level should be set to > Z_DEFAULT_COMPRESSION if there is nothing set in the compression > spec for a zlib build. pg_receivewal.c enforces that already.
So, I have looked at this one. And it seems to me that the confusion comes down to the existence of PG_COMPRESSION_OPTION_LEVEL. I have considered a couple of approaches here, like introducing an extra routine in compression.c to assign a default compression level, but my conclusion is at the end simpler: we always finish by setting up a level even if the caller wants nothing, in which can we can just use each library's default. And lz4, zstd and zlib are able to handle the case where a default is given down to their internal routines just fine. Attached is the patch I am finishing with, consisting of: - the removal of PG_COMPRESSION_OPTION_LEVEL. - assigning a default compression level when nothing is specified in the spec. - a couple of complifications in pg_receivewal, pg_basebackup and the backend code as there is no need to worry about the compression level. A nice effect of this approach is that we can centralize the checks on lz4, zstd and zlib when a build does not support any of these options, as well as centralize the place where the default compression levels are set. This passes all the regression tests, and it fixes the issue reported. (Note that I have yet to run tests with all the libraries disabled in ./configure.) Thoughts? -- Michael
From 8752cb283d65e40fc4096f84d99bcb93d97c59bc Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
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 <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
+ * 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
signature.asc
Description: PGP signature
