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 protected]>
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
