0001-Simplify-compression-level-handling-in-compression-s.patch

From 8752cb283d65e40fc4096f84d99bcb93d97c59bc Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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