Date: Wed, 16 Mar 2022 10:12:54 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Jeevan Ladhe <jeevanladhe.os@gmail.com>,
	Dipesh Pandit <dipesh.pandit@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	"Shinoda, Noriyoshi (PN Japan FSIP)" <noriyoshi.shinoda@hpe.com>,
	Abhijit Menon-Sen <ams@toroid.org>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>,
	Mark Dilger <mark.dilger@enterprisedb.com>,
	pgsql-hackers@postgresql.org,
	tushar <tushar.ahuja@enterprisedb.com>
Subject: Re: refactoring basebackup.c (zstd negative compression)
Message-ID: <20220316151253.GB28503@telsasoft.com>
References: <CANm22Ciseat_kJwY_vixu6Wen48E7EVV5E+tWzZ9qN7SYJYOfQ@mail.gmail.com>
 <CA+TgmoZZmZctarpJyRiLNcePeTUS0z-Y75tAPUnFYQvRcj1Xzw@mail.gmail.com>
 <CANm22CiVwmix7JGOGhJYsbNt9M8AH-svG2HQfPP=MdZ2kMHfzw@mail.gmail.com>
 <20220311010223.GI28503@telsasoft.com>
 <CA+Tgmob6Rnjz-Qv32h3yJn8nnUkLhrtQDAS4y5AtsgtorAFHRA@mail.gmail.com>
 <20220312015253.zokhkwdn5xbbd4vf@alap3.anarazel.de>
 <CA+TgmoZ10QPYaUxe8+RfJT5ptyDe-ML=oE_FyyFa+sNS2GBgZw@mail.gmail.com>
 <CAN1g5_F0KBgitEVLO8T0ynaBcQBfRMCjH9wKguDJZpX1hF3gFA@mail.gmail.com>
 <CANm22Ch_hU-86CCC-HkUAOY+6jYfGp5jJkde97fTu3u7u=PW9g@mail.gmail.com>
 <CA+Tgmobdhp4V5p4Lam2scFVPr+zsm4tNcOWikZJ4LV9S=CKSxw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="qD3brAgIG4LbUq6d"
Content-Disposition: inline
In-Reply-To: <CA+Tgmobdhp4V5p4Lam2scFVPr+zsm4tNcOWikZJ4LV9S=CKSxw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20220316151253.GB28503%40telsasoft.com>
Precedence: bulk


--qD3brAgIG4LbUq6d
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--qD3brAgIG4LbUq6d
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-pg_basebackup-support-Zstd-negative-compression-leve.patch"

From 9a330a3a1801352cef3b5912e31aba61760dac32 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
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


--qD3brAgIG4LbUq6d--