Subject: Re: refactoring basebackup.c (zstd negative compression)

From: Justin Pryzby <[email-masked]>
Date: Wed, 16 Mar 2022 10:12:54 -0500

Should zstd's negative compression levels be supported here ?

Here's a POC patch which is enough to play with it.

$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd |wc -c
12305659
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:1 |wc -c
13827521
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:0 |wc -c
12304018
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:-1 |wc -c
16443893
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:-2 |wc -c
17349563
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:-4 |wc -c
19452631
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=zstd:-7 |wc -c
21871505

Also, with a partial regression DB, this crashes when writing to stdout.

$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=lz4 |wc -c
pg_basebackup: bbstreamer_lz4.c:172: bbstreamer_lz4_compressor_content: Assertion `mystreamer->base.bbs_buffer.maxlen >= out_bound' failed.
24117248

#4  0x000055555555e8b4 in bbstreamer_lz4_compressor_content (streamer=0x5555555a5260, member=0x7fffffffc760, 
    data=0x7ffff3068010 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n\"Files\": [\n{ \"Path\": \"backup_label\", \"Size\": 227, \"Last-Modified\": \"2022-03-16 02:29:11 GMT\", \"Checksum-Algorithm\": \"CRC32C\", \"Checksum\": \"46f69d99\" },\n{ \"Pa"..., len=401072, context=BBSTREAMER_MEMBER_CONTENTS) at bbstreamer_lz4.c:172
        mystreamer = 0x5555555a5260
        next_in = 0x7ffff3068010 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n\"Files\": [\n{ \"Path\": \"backup_label\", \"Size\": 227, \"Last-Modified\": \"2022-03-16 02:29:11 GMT\", \"Checksum-Algorithm\": \"CRC32C\", \"Checksum\": \"46f69d99\" },\n{ \"Pa"...
        ...

(gdb) p mystreamer->base.bbs_buffer.maxlen
$1 = 524288
(gdb) p (int) LZ4F_compressBound(len, &mystreamer->prefs)
$4 = 524300

This is with: liblz4-1:amd64 1.9.2-2ubuntu0.20.04.1
>From 9a330a3a1801352cef3b5912e31aba61760dac32 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Thu, 10 Mar 2022 20:16:19 -0600
Subject: [PATCH] pg_basebackup: support Zstd negative compression levels

"higher than maximum" is bogus

TODO: each compression methods should enforce its own levels
---
 src/backend/replication/basebackup_zstd.c |  8 ++++++--
 src/backend/replication/repl_scanner.l    |  4 +++-
 src/bin/pg_basebackup/bbstreamer_zstd.c   |  7 ++++++-
 src/bin/pg_basebackup/pg_basebackup.c     | 23 ++++++++++++-----------
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/src/backend/replication/basebackup_zstd.c b/src/backend/replication/basebackup_zstd.c
index c0e2be6e27b..4464fcb30e1 100644
--- a/src/backend/replication/basebackup_zstd.c
+++ b/src/backend/replication/basebackup_zstd.c
@@ -71,7 +71,7 @@ bbsink_zstd_new(bbsink *next, int compresslevel)
 
 	Assert(next != NULL);
 
-	if (compresslevel < 0 || compresslevel > 22)
+	if (compresslevel < -7 || compresslevel > 22)
 		ereport(ERROR,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("zstd compression level %d is out of range",
@@ -96,13 +96,17 @@ bbsink_zstd_begin_backup(bbsink *sink)
 {
 	bbsink_zstd *mysink = (bbsink_zstd *) sink;
 	size_t		output_buffer_bound;
+	size_t		ret;
 
 	mysink->cctx = ZSTD_createCCtx();
 	if (!mysink->cctx)
 		elog(ERROR, "could not create zstd compression context");
 
-	ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
+	ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
 						   mysink->compresslevel);
+	if (ZSTD_isError(ret))
+		elog(ERROR, "could not create zstd compression context: %s",
+				ZSTD_getErrorName(ret));
 
 	/*
 	 * We need our own buffer, because we're going to pass different data to
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index 4b64c0d768b..05c4ef463a1 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -86,6 +86,7 @@ xdinside		[^"]+
 
 digit			[0-9]
 hexdigit		[0-9A-Fa-f]
+sign			("-"|"+")
 
 ident_start		[A-Za-z\200-\377_]
 ident_cont		[A-Za-z\200-\377_0-9\$]
@@ -127,9 +128,10 @@ NOEXPORT_SNAPSHOT	{ return K_NOEXPORT_SNAPSHOT; }
 USE_SNAPSHOT		{ return K_USE_SNAPSHOT; }
 WAIT				{ return K_WAIT; }
 
+
 {space}+		{ /* do nothing */ }
 
-{digit}+		{
+{sign}?{digit}+		{
 					yylval.uintval = strtoul(yytext, NULL, 10);
 					return UCONST;
 				}
diff --git a/src/bin/pg_basebackup/bbstreamer_zstd.c b/src/bin/pg_basebackup/bbstreamer_zstd.c
index e86749a8fb7..337e789b6a1 100644
--- a/src/bin/pg_basebackup/bbstreamer_zstd.c
+++ b/src/bin/pg_basebackup/bbstreamer_zstd.c
@@ -67,6 +67,7 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, int compresslevel)
 {
 #ifdef USE_ZSTD
 	bbstreamer_zstd_frame *streamer;
+	size_t ret;
 
 	Assert(next != NULL);
 
@@ -84,9 +85,13 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, int compresslevel)
 		pg_log_error("could not create zstd compression context");
 
 	/* Initialize stream compression preferences */
-	ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
+	ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
 						   compresslevel);
 
+	if (ZSTD_isError(ret))
+		pg_log_error("could not create zstd compression context: %s",
+				ZSTD_getErrorName(ret));
+
 	/* Initialize the ZSTD output buffer. */
 	streamer->zstd_outBuf.dst = streamer->base.bbs_buffer.data;
 	streamer->zstd_outBuf.size = streamer->base.bbs_buffer.maxlen;
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2943d9ec1a0..2db600a34be 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1129,7 +1129,7 @@ parse_compress_options(char *src, WalCompressionMethod *methodres,
 	 * For any of the methods currently supported, the data after the
 	 * separator can just be an integer.
 	 */
-	if (!option_parse_int(sep, "-Z/--compress", 0, INT_MAX,
+	if (!option_parse_int(sep, "-Z/--compress", -7, INT_MAX,
 						  levelres))
 		exit(1);
 
@@ -2079,7 +2079,7 @@ BaseBackup(void)
 		}
 		AppendStringCommandOption(&buf, use_new_option_syntax,
 								  "COMPRESSION", compressmethodstr);
-		if (compresslevel >= 1) /* not 0 or Z_DEFAULT_COMPRESSION */
+		if (compresslevel != 0) /* not 0 or Z_DEFAULT_COMPRESSION */
 			AppendIntegerCommandOption(&buf, use_new_option_syntax,
 									   "COMPRESSION_LEVEL", compresslevel);
 	}
@@ -2896,10 +2896,10 @@ main(int argc, char **argv)
 			}
 			break;
 		case COMPRESSION_GZIP:
-			if (compresslevel > 9)
+			if (compresslevel > 9 || compresslevel < -1) /* Z_DEFAULT_COMPRESSION */
 			{
-				pg_log_error("compression level %d of method %s higher than maximum of 9",
-							 compresslevel, "gzip");
+				pg_log_error("compression level %d of method %s out of range (%s)",
+							 compresslevel, "gzip", "1..9");
 				exit(1);
 			}
 			if (compressloc == COMPRESS_LOCATION_CLIENT)
@@ -2915,18 +2915,19 @@ main(int argc, char **argv)
 			}
 			break;
 		case COMPRESSION_LZ4:
-			if (compresslevel > 12)
+			if (compresslevel > 12 || compresslevel < 0)
 			{
-				pg_log_error("compression level %d of method %s higher than maximum of 12",
-							 compresslevel, "lz4");
+				pg_log_error("compression level %d of method %s out of range (%s)",
+							 compresslevel, "lz4", "1..12");
 				exit(1);
 			}
 			break;
 		case COMPRESSION_ZSTD:
-			if (compresslevel > 22)
+			break; // XXX
+			if (compresslevel > 22 || compresslevel < -7)
 			{
-				pg_log_error("compression level %d of method %s higher than maximum of 22",
-							 compresslevel, "zstd");
+				pg_log_error("compression level %d of method %s out of range (%s)",
+							 compresslevel, "zstd", "-7..22");
 				exit(1);
 			}
 			break;
-- 
2.17.1