public inbox for [email protected]
help / color / mirror / Atom feedFrom: Justin Pryzby <[email protected]>
To: Robert Haas <[email protected]>
Cc: Jeevan Ladhe <[email protected]>
Cc: Dipesh Pandit <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Shinoda, Noriyoshi (PN Japan FSIP) <[email protected]>
Cc: Abhijit Menon-Sen <[email protected]>
Cc: Dmitry Dolgov <[email protected]>
Cc: Jeevan Ladhe <[email protected]>
Cc: Mark Dilger <[email protected]>
Cc: [email protected]
Cc: tushar <[email protected]>
Subject: Re: refactoring basebackup.c (zstd negative compression)
Date: Wed, 16 Mar 2022 10:12:54 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+Tgmobdhp4V5p4Lam2scFVPr+zsm4tNcOWikZJ4LV9S=CKSxw@mail.gmail.com>
References: <CANm22Ciseat_kJwY_vixu6Wen48E7EVV5E+tWzZ9qN7SYJYOfQ@mail.gmail.com>
<CA+TgmoZZmZctarpJyRiLNcePeTUS0z-Y75tAPUnFYQvRcj1Xzw@mail.gmail.com>
<CANm22CiVwmix7JGOGhJYsbNt9M8AH-svG2HQfPP=MdZ2kMHfzw@mail.gmail.com>
<[email protected]>
<CA+Tgmob6Rnjz-Qv32h3yJn8nnUkLhrtQDAS4y5AtsgtorAFHRA@mail.gmail.com>
<[email protected]>
<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>
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
Attachments:
[text/x-diff] 0001-pg_basebackup-support-Zstd-negative-compression-leve.patch (5.7K, ../[email protected]/2-0001-pg_basebackup-support-Zstd-negative-compression-leve.patch)
download | inline diff:
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
view thread (2+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: refactoring basebackup.c (zstd negative compression)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox