Skip to content

Commit f352e2d

Browse files
michaelpqmichaelpq
committed
Simplify handling of compression level with compression specifications
PG_COMPRESSION_OPTION_LEVEL is removed from the compression specification logic, and instead the compression level is always assigned with each library's default if nothing is directly given. This centralizes the checks on the compression methods supported by a given build, and always assigns a default compression level when parsing a compression specification. This results in complaining at an earlier stage than previously if a build supports a compression method or not, aka when parsing a specification in the backend or the frontend, and not when processing it. zstd, lz4 and zlib are able to handle in their respective routines setting up the compression level the case of a default value, hence the backend or frontend code (pg_receivewal or pg_basebackup) has now no need to know what the default compression level should be if nothing is specified: the logic is now done so as the specification parsing assigns it. It can also be enforced by passing down a "level" set to the default value, that the backend will accept (the replication protocol is for example able to handle a command like BASE_BACKUP (COMPRESSION_DETAIL 'gzip:level=-1')). This code simplification fixes an issue with pg_basebackup --gzip introduced by ffd5365, where the tarball of the streamed WAL segments would be created as of pg_wal.tar.gz with uncompressed contents, while the intention is to compress the segments with gzip at a default level. The origin of the confusion comes from the handling of the default compression level of gzip (-1 or Z_DEFAULT_COMPRESSION) and the value of 0 was getting assigned, which is what walmethods.c would consider as equivalent to no compression when streaming WAL segments with its tar methods. Assigning always the compression level removes the confusion of some code paths considering a value of 0 set in a specification as either no compression or a default compression level. Note that 010_pg_basebackup.pl has to be adjusted to skip a few tests where the shape of the compression detail string for client and server-side compression was checked using gzip. This is a result of the code simplification, as gzip specifications cannot be used if a build does not support it. Reported-by: Tom Lane Reviewed-by: Tom Lane Discussion: https://postgr.es/m/1400032.1662217889@sss.pgh.pa.us Backpatch-through: 15
1 parent 0a20ff5 commit f352e2d

File tree

12 files changed

+189
-159
lines changed

12 files changed

+189
-159
lines changed

‎doc/src/sgml/protocol.sgml‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2752,9 +2752,13 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
27522752
<para>
27532753
The <literal>level</literal> keyword sets the compression level.
27542754
For <literal>gzip</literal> the compression level should be an
2755-
integer between 1 and 9, for <literal>lz4</literal> an integer
2756-
between 1 and 12, and for <literal>zstd</literal> an integer
2757-
between 1 and 22.
2755+
integer between <literal>1</literal> and <literal>9</literal>
2756+
(default <literal>Z_DEFAULT_COMPRESSION</literal> or
2757+
<literal>-1</literal>), for <literal>lz4</literal> an integer
2758+
between 1 and 12 (default <literal>0</literal> for fast compression
2759+
mode), and for <literal>zstd</literal> an integer between
2760+
<literal>1</literal> and <literal>22</literal> (default
2761+
<literal>ZSTD_CLEVEL_DEFAULT</literal> or <literal>3</literal>).
27582762
</para>
27592763

27602764
<para>

‎src/backend/backup/basebackup_gzip.c‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,9 @@ bbsink_gzip_new(bbsink *next, pg_compress_specification *compress)
7272

7373
Assert(next != NULL);
7474

75-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
76-
compresslevel = Z_DEFAULT_COMPRESSION;
77-
else
78-
{
79-
compresslevel = compress->level;
80-
Assert(compresslevel >= 1 && compresslevel <= 9);
81-
}
75+
compresslevel = compress->level;
76+
Assert((compresslevel >= 1 && compresslevel <= 9) ||
77+
compresslevel == Z_DEFAULT_COMPRESSION);
8278

8379
sink = palloc0(sizeof(bbsink_gzip));
8480
*((const bbsink_ops **) &sink->base.bbs_ops) = &bbsink_gzip_ops;

‎src/backend/backup/basebackup_lz4.c‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,8 @@ bbsink_lz4_new(bbsink *next, pg_compress_specification *compress)
7272

7373
Assert(next != NULL);
7474

75-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) == 0)
76-
compresslevel = 0;
77-
else
78-
{
79-
compresslevel = compress->level;
80-
Assert(compresslevel >= 1 && compresslevel <= 12);
81-
}
75+
compresslevel = compress->level;
76+
Assert(compresslevel >= 0 && compresslevel <= 12);
8277

8378
sink = palloc0(sizeof(bbsink_lz4));
8479
*((const bbsink_ops **) &sink->base.bbs_ops) = &bbsink_lz4_ops;

‎src/backend/backup/basebackup_zstd.c‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,11 @@ bbsink_zstd_begin_backup(bbsink *sink)
9696
if (!mysink->cctx)
9797
elog(ERROR, "could not create zstd compression context");
9898

99-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
100-
{
101-
ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
102-
compress->level);
103-
if (ZSTD_isError(ret))
104-
elog(ERROR, "could not set zstd compression level to %d: %s",
105-
compress->level, ZSTD_getErrorName(ret));
106-
}
99+
ret = ZSTD_CCtx_setParameter(mysink->cctx, ZSTD_c_compressionLevel,
100+
compress->level);
101+
if (ZSTD_isError(ret))
102+
elog(ERROR, "could not set zstd compression level to %d: %s",
103+
compress->level, ZSTD_getErrorName(ret));
107104

108105
if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)
109106
{

‎src/bin/pg_basebackup/bbstreamer_gzip.c‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,7 @@ bbstreamer_gzip_writer_new(char *pathname, FILE *file,
107107
pg_fatal("could not open output file: %m");
108108
}
109109

110-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0 &&
111-
gzsetparams(streamer->gzfile, compress->level,
112-
Z_DEFAULT_STRATEGY) != Z_OK)
110+
if (gzsetparams(streamer->gzfile, compress->level, Z_DEFAULT_STRATEGY) != Z_OK)
113111
pg_fatal("could not set compression level %d: %s",
114112
compress->level, get_gz_error(streamer->gzfile));
115113

‎src/bin/pg_basebackup/bbstreamer_lz4.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,7 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, pg_compress_specification *compr
8888
prefs = &streamer->prefs;
8989
memset(prefs, 0, sizeof(LZ4F_preferences_t));
9090
prefs->frameInfo.blockSizeID = LZ4F_max256KB;
91-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
92-
prefs->compressionLevel = compress->level;
91+
prefs->compressionLevel = compress->level;
9392

9493
ctxError = LZ4F_createCompressionContext(&streamer->cctx, LZ4F_VERSION);
9594
if (LZ4F_isError(ctxError))

‎src/bin/pg_basebackup/bbstreamer_zstd.c‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,12 @@ bbstreamer_zstd_compressor_new(bbstreamer *next, pg_compress_specification *comp
8484
if (!streamer->cctx)
8585
pg_fatal("could not create zstd compression context");
8686

87-
/* Set compression level, if specified */
88-
if ((compress->options & PG_COMPRESSION_OPTION_LEVEL) != 0)
89-
{
90-
ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
91-
compress->level);
92-
if (ZSTD_isError(ret))
93-
pg_fatal("could not set zstd compression level to %d: %s",
94-
compress->level, ZSTD_getErrorName(ret));
95-
}
87+
/* Set compression level */
88+
ret = ZSTD_CCtx_setParameter(streamer->cctx, ZSTD_c_compressionLevel,
89+
compress->level);
90+
if (ZSTD_isError(ret))
91+
pg_fatal("could not set zstd compression level to %d: %s",
92+
compress->level, ZSTD_getErrorName(ret));
9693

9794
/* Set # of workers, if specified */
9895
if ((compress->options & PG_COMPRESSION_OPTION_WORKERS) != 0)

‎src/bin/pg_basebackup/pg_basebackup.c‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2012,9 +2012,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
20122012
if (client_compress->algorithm == PG_COMPRESSION_GZIP)
20132013
{
20142014
wal_compress_algorithm = PG_COMPRESSION_GZIP;
2015-
wal_compress_level =
2016-
(client_compress->options & PG_COMPRESSION_OPTION_LEVEL)
2017-
!= 0 ? client_compress->level : 0;
2015+
wal_compress_level = client_compress->level;
20182016
}
20192017
else
20202018
{
@@ -2023,7 +2021,8 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
20232021
}
20242022

20252023
StartLogStreamer(xlogstart, starttli, sysidentifier,
2026-
wal_compress_algorithm, wal_compress_level);
2024+
wal_compress_algorithm,
2025+
wal_compress_level);
20272026
}
20282027

20292028
if (serverMajor >= 1500)

‎src/bin/pg_basebackup/pg_receivewal.c‎

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -875,38 +875,11 @@ main(int argc, char **argv)
875875
pg_fatal("invalid compression specification: %s",
876876
error_detail);
877877

878-
/* Extract the compression level, if found in the specification */
879-
if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) != 0)
880-
compresslevel = compression_spec.level;
881-
882-
switch (compression_algorithm)
883-
{
884-
case PG_COMPRESSION_NONE:
885-
/* nothing to do */
886-
break;
887-
case PG_COMPRESSION_GZIP:
888-
#ifdef HAVE_LIBZ
889-
if ((compression_spec.options & PG_COMPRESSION_OPTION_LEVEL) == 0)
890-
{
891-
pg_log_info("no value specified for --compress, switching to default");
892-
compresslevel = Z_DEFAULT_COMPRESSION;
893-
}
894-
#else
895-
pg_fatal("this build does not support compression with %s",
896-
"gzip");
897-
#endif
898-
break;
899-
case PG_COMPRESSION_LZ4:
900-
#ifndef USE_LZ4
901-
pg_fatal("this build does not support compression with %s",
902-
"LZ4");
903-
#endif
904-
break;
905-
case PG_COMPRESSION_ZSTD:
906-
pg_fatal("compression with %s is not yet supported", "ZSTD");
907-
break;
908-
}
878+
/* Extract the compression level */
879+
compresslevel = compression_spec.level;
909880

881+
if (compression_algorithm == PG_COMPRESSION_ZSTD)
882+
pg_fatal("compression with %s is not yet supported", "ZSTD");
910883

911884
/*
912885
* Check existence of destination folder.

‎src/bin/pg_basebackup/t/010_pg_basebackup.pl‎

Lines changed: 72 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -86,71 +86,81 @@
8686
# Now that we have a server that supports replication commands, test whether
8787
# certain invalid compression commands fail on the client side with client-side
8888
# compression and on the server side with server-side compression.
89-
my $client_fails = 'pg_basebackup: error: ';
90-
my $server_fails =
91-
'pg_basebackup: error: could not initiate base backup: ERROR: ';
92-
my @compression_failure_tests = (
93-
[
94-
'extrasquishy',
95-
'unrecognized compression algorithm "extrasquishy"',
96-
'failure on invalid compression algorithm'
97-
],
98-
[
99-
'gzip:',
100-
'invalid compression specification: found empty string where a compression option was expected',
101-
'failure on empty compression options list'
102-
],
103-
[
104-
'gzip:thunk',
105-
'invalid compression specification: unknown compression option "thunk"',
106-
'failure on unknown compression option'
107-
],
108-
[
109-
'gzip:level',
110-
'invalid compression specification: compression option "level" requires a value',
111-
'failure on missing compression level'
112-
],
113-
[
114-
'gzip:level=',
115-
'invalid compression specification: value for compression option "level" must be an integer',
116-
'failure on empty compression level'
117-
],
118-
[
119-
'gzip:level=high',
120-
'invalid compression specification: value for compression option "level" must be an integer',
121-
'failure on non-numeric compression level'
122-
],
123-
[
124-
'gzip:level=236',
125-
'invalid compression specification: compression algorithm "gzip" expects a compression level between 1 and 9',
126-
'failure on out-of-range compression level'
127-
],
128-
[
129-
'gzip:level=9,',
130-
'invalid compression specification: found empty string where a compression option was expected',
131-
'failure on extra, empty compression option'
132-
],
133-
[
134-
'gzip:workers=3',
135-
'invalid compression specification: compression algorithm "gzip" does not accept a worker count',
136-
'failure on worker count for gzip'
137-
],);
138-
for my $cft (@compression_failure_tests)
89+
SKIP:
13990
{
140-
my $cfail = quotemeta($client_fails . $cft->[1]);
141-
my $sfail = quotemeta($server_fails . $cft->[1]);
142-
$node->command_fails_like(
143-
[ 'pg_basebackup', '-D', "$tempdir/backup", '--compress', $cft->[0] ],
144-
qr/$cfail/,
145-
'client ' . $cft->[2]);
146-
$node->command_fails_like(
91+
skip "postgres was not built with ZLIB support", 6
92+
if (!check_pg_config("#define HAVE_LIBZ 1"));
93+
94+
my $client_fails = 'pg_basebackup: error: ';
95+
my $server_fails =
96+
'pg_basebackup: error: could not initiate base backup: ERROR: ';
97+
my @compression_failure_tests = (
14798
[
148-
'pg_basebackup', '-D',
149-
"$tempdir/backup", '--compress',
150-
'server-' . $cft->[0]
99+
'extrasquishy',
100+
'unrecognized compression algorithm "extrasquishy"',
101+
'failure on invalid compression algorithm'
151102
],
152-
qr/$sfail/,
153-
'server ' . $cft->[2]);
103+
[
104+
'gzip:',
105+
'invalid compression specification: found empty string where a compression option was expected',
106+
'failure on empty compression options list'
107+
],
108+
[
109+
'gzip:thunk',
110+
'invalid compression specification: unknown compression option "thunk"',
111+
'failure on unknown compression option'
112+
],
113+
[
114+
'gzip:level',
115+
'invalid compression specification: compression option "level" requires a value',
116+
'failure on missing compression level'
117+
],
118+
[
119+
'gzip:level=',
120+
'invalid compression specification: value for compression option "level" must be an integer',
121+
'failure on empty compression level'
122+
],
123+
[
124+
'gzip:level=high',
125+
'invalid compression specification: value for compression option "level" must be an integer',
126+
'failure on non-numeric compression level'
127+
],
128+
[
129+
'gzip:level=236',
130+
'invalid compression specification: compression algorithm "gzip" expects a compression level between 1 and 9',
131+
'failure on out-of-range compression level'
132+
],
133+
[
134+
'gzip:level=9,',
135+
'invalid compression specification: found empty string where a compression option was expected',
136+
'failure on extra, empty compression option'
137+
],
138+
[
139+
'gzip:workers=3',
140+
'invalid compression specification: compression algorithm "gzip" does not accept a worker count',
141+
'failure on worker count for gzip'
142+
],);
143+
for my $cft (@compression_failure_tests)
144+
{
145+
my $cfail = quotemeta($client_fails . $cft->[1]);
146+
my $sfail = quotemeta($server_fails . $cft->[1]);
147+
$node->command_fails_like(
148+
[
149+
'pg_basebackup', '-D',
150+
"$tempdir/backup", '--compress',
151+
$cft->[0]
152+
],
153+
qr/$cfail/,
154+
'client ' . $cft->[2]);
155+
$node->command_fails_like(
156+
[
157+
'pg_basebackup', '-D',
158+
"$tempdir/backup", '--compress',
159+
'server-' . $cft->[0]
160+
],
161+
qr/$sfail/,
162+
'server ' . $cft->[2]);
163+
}
154164
}
155165

156166
# Write some files to test that they are not copied.

0 commit comments

Comments
 (0)