On Tue, Jan 18, 2022 at 10:04:56AM -0500, Robert Haas wrote:
> I think it could make sense for you implement
> --compress=METHOD[:LEVEL], keeping -z and -Z N as synonyms for
> --compress=gzip and --compress=gzip:N, and with --compress=N being
> interpreted as --compress=gzip:N. Then I'll generalize that to
> --compress=[{client|server}-]METHOD[:LEVEL] on the other thread. I
> think we should leave it where, if you don't say either client or
> server, you get client, because that's the historical behavior.
>
> If that doesn't work for you, please let me know what you would prefer.WFM. Attached is a patch that extends --compress to handle a method with an optional compression level. Some extra tests are added to cover all that. Thoughts? -- Michael
From abf0848b0975f5a33ecaee725ab616b92b914820 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 19 Jan 2022 13:27:00 +0900
Subject: [PATCH v5] Rework the option set of pg_basebackup
---
src/bin/pg_basebackup/pg_basebackup.c | 138 ++++++++++++++++---
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 46 ++++++-
doc/src/sgml/ref/pg_basebackup.sgml | 21 ++-
3 files changed, 182 insertions(+), 23 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2a58be638a..734fab4e43 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -123,6 +123,7 @@ static bool showprogress = false;
static bool estimatesize = true;
static int verbose = 0;
static int compresslevel = 0;
+static WalCompressionMethod compressmethod = COMPRESSION_NONE;
static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
@@ -376,7 +377,8 @@ usage(void)
printf(_(" -X, --wal-method=none|fetch|stream\n"
" include required WAL files with specified method\n"));
printf(_(" -z, --gzip compress tar output\n"));
- printf(_(" -Z, --compress=0-9 compress tar output with given compression level\n"));
+ printf(_(" -Z, --compress=[{gzip,none}[:LEVEL] or [LEVEL]\n"
+ " compress tar output with given compression method or level\n"));
printf(_("\nGeneral options:\n"));
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
@@ -541,8 +543,7 @@ LogStreamerMain(logstreamer_param *param)
stream.do_sync);
else
stream.walmethod = CreateWalTarMethod(param->xlog,
- (compresslevel != 0) ?
- COMPRESSION_GZIP : COMPRESSION_NONE,
+ compressmethod,
compresslevel,
stream.do_sync);
@@ -933,6 +934,78 @@ parse_max_rate(char *src)
return (int32) result;
}
+/*
+ * Utility wrapper to parse the values specified for -Z/--compress.
+ * *methodres and *levelres will be optionally filled with values coming
+ * from the parsed results.
+ */
+static void
+parse_compress_options(char *src, WalCompressionMethod *methodres,
+ int *levelres)
+{
+ /* First check after the compression method */
+ if (strncmp(src, "gzip", 4) == 0 ||
+ strncmp(src, "none", 4) == 0)
+ {
+ char *p = src;
+
+ if (strncmp(src, "gzip", 4) == 0)
+ {
+ *methodres = COMPRESSION_GZIP;
+ p += 4;
+ }
+ else if (strncmp(src, "none", 4) == 0)
+ {
+ *methodres = COMPRESSION_NONE;
+ p += 4;
+ }
+
+ /* If the option has no more data, we are done */
+ if (*p == '\0')
+ return;
+
+ /*
+ * Specifying the level is optional, and it should be prefixed
+ * by a colon.
+ */
+ if (*p != ':')
+ {
+ pg_log_error("invalid separator found to specify compression level: expected ':' but found '%c'",
+ *p);
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+ p++;
+
+ if (*p == '\0')
+ {
+ pg_log_error("empty value for option %s", "-Z/--compress");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+
+ if (!option_parse_int(p, "-Z/--compress", 0, 9,
+ levelres))
+ exit(1);
+ return;
+ }
+
+ /*
+ * If the beginning of the option value does not match any of the methods
+ * supported, check for an integer. This is backward-compatible with the
+ * pre-14 behavior where a caller could define a compression level of
+ * 0 for no compression, and of [1,9] for gzip compression.
+ */
+ if (!option_parse_int(src, "-Z/--compress", 0, 9,
+ levelres))
+ exit(1);
+
+ *methodres = (*levelres > 0) ?
+ COMPRESSION_GZIP : COMPRESSION_NONE;
+}
+
/*
* Read a stream of COPY data and invoke the provided callback for each
* chunk.
@@ -993,7 +1066,7 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
bool is_recovery_guc_supported,
bool expect_unterminated_tarfile)
{
- bbstreamer *streamer;
+ bbstreamer *streamer = NULL;
bbstreamer *manifest_inject_streamer = NULL;
bool inject_manifest;
bool must_parse_archive;
@@ -1052,19 +1125,22 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
archive_file = NULL;
}
+ if (compressmethod == COMPRESSION_NONE)
+ streamer = bbstreamer_plain_writer_new(archive_filename,
+ archive_file);
#ifdef HAVE_LIBZ
- if (compresslevel != 0)
+ else if (compressmethod == COMPRESSION_GZIP)
{
strlcat(archive_filename, ".gz", sizeof(archive_filename));
streamer = bbstreamer_gzip_writer_new(archive_filename,
archive_file,
compresslevel);
}
- else
#endif
- streamer = bbstreamer_plain_writer_new(archive_filename,
- archive_file);
-
+ else
+ {
+ Assert(false); /* not reachable */
+ }
/*
* If we need to parse the archive for whatever reason, then we'll
@@ -2220,11 +2296,11 @@ main(int argc, char **argv)
#else
compresslevel = 1; /* will be rejected below */
#endif
+ compressmethod = COMPRESSION_GZIP;
break;
case 'Z':
- if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
- &compresslevel))
- exit(1);
+ parse_compress_options(optarg, &compressmethod,
+ &compresslevel);
break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
@@ -2321,7 +2397,7 @@ main(int argc, char **argv)
/*
* Mutually exclusive arguments
*/
- if (format == 'p' && compresslevel != 0)
+ if (format == 'p' && compressmethod != COMPRESSION_NONE)
{
pg_log_error("only tar mode backups can be compressed");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
@@ -2399,13 +2475,39 @@ main(int argc, char **argv)
}
}
-#ifndef HAVE_LIBZ
- if (compresslevel != 0)
+ /*
+ * Compression-related checks.
+ */
+ switch (compressmethod)
{
- pg_log_error("this build does not support compression");
- exit(1);
- }
+ case COMPRESSION_NONE:
+ if (compresslevel != 0)
+ {
+ pg_log_error("cannot use compression level with method %s",
+ "none");
+ fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
+ progname);
+ exit(1);
+ }
+ break;
+ case COMPRESSION_GZIP:
+#ifdef HAVE_LIBZ
+ if (compresslevel == 0)
+ {
+ pg_log_info("no value specified for compression level, switching to default");
+ compresslevel = Z_DEFAULT_COMPRESSION;
+ }
+#else
+ pg_log_error("this build does not support compression with %s",
+ "gzip");
+ exit(1);
#endif
+ break;
+ case COMPRESSION_LZ4:
+ /* option not supported */
+ Assert(false);
+ break;
+ }
if (showprogress && !estimatesize)
{
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index f0243f28d4..0ddbbee818 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -10,7 +10,7 @@ use File::Path qw(rmtree);
use Fcntl qw(:seek);
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
-use Test::More tests => 115;
+use Test::More tests => 123;
program_help_ok('pg_basebackup');
program_version_ok('pg_basebackup');
@@ -38,6 +38,32 @@ my $pgdata = $node->data_dir;
$node->command_fails(['pg_basebackup'],
'pg_basebackup needs target directory specified');
+# Sanity checks for options
+$node->command_fails_like(
+ [
+ 'pg_basebackup', '-D',
+ "$tempdir/backup", '--compress',
+ 'none:1'
+ ],
+ qr/\Qpg_basebackup: error: cannot use compression level with method none/,
+ 'failure if method "none" specified with compression level');
+$node->command_fails_like(
+ [
+ 'pg_basebackup', '-D',
+ "$tempdir/backup", '--compress',
+ 'none+'
+ ],
+ qr/\Qpg_basebackup: error: invalid separator found to specify compression level/,
+ 'failure on incorrect separator to define compression level');
+$node->command_fails_like(
+ [
+ 'pg_basebackup', '-D',
+ "$tempdir/backup", '--compress',
+ 'none:'
+ ],
+ qr/\Qpg_basebackup: error: empty value for option/,
+ 'failure on missing compression level value');
+
# Some Windows ANSI code pages may reject this filename, in which case we
# quietly proceed without this bit of test coverage.
if (open my $badchars, '>>', "$tempdir/pgdata/FOO\xe0\xe0\xe0BAR")
@@ -637,7 +663,7 @@ note "Testing pg_basebackup with compression methods";
# Check ZLIB compression if available.
SKIP:
{
- skip "postgres was not built with ZLIB support", 5
+ skip "postgres was not built with ZLIB support", 7
if (!check_pg_config("#define HAVE_LIBZ 1"));
$node->command_ok(
@@ -655,15 +681,26 @@ SKIP:
'--format', 't'
],
'pg_basebackup with --gzip');
+ $node->command_ok(
+ [
+ @pg_basebackup_defs, '-D',
+ "$tempdir/backup_gzip3", '--compress',
+ 'gzip:1',
+ '--format', 't'
+ ],
+ 'pg_basebackup with --compress=gzip:1');
# Verify that the stored files are generated with their expected
# names.
my @zlib_files = glob "$tempdir/backup_gzip/*.tar.gz";
is(scalar(@zlib_files), 2,
- "two files created with --compress (base.tar.gz and pg_wal.tar.gz)");
+ "two files created with --compress=NUM (base.tar.gz and pg_wal.tar.gz)");
my @zlib_files2 = glob "$tempdir/backup_gzip2/*.tar.gz";
is(scalar(@zlib_files2), 2,
"two files created with --gzip (base.tar.gz and pg_wal.tar.gz)");
+ my @zlib_files3 = glob "$tempdir/backup_gzip3/*.tar.gz";
+ is(scalar(@zlib_files3), 2,
+ "two files created with --compress=gzip:NUM (base.tar.gz and pg_wal.tar.gz)");
# Check the integrity of the files generated.
my $gzip = $ENV{GZIP_PROGRAM};
@@ -673,8 +710,9 @@ SKIP:
|| system_log($gzip, '--version') != 0);
my $gzip_is_valid =
- system_log($gzip, '--test', @zlib_files, @zlib_files2);
+ system_log($gzip, '--test', @zlib_files, @zlib_files2, @zlib_files3);
is($gzip_is_valid, 0, "gzip verified the integrity of compressed data");
rmtree("$tempdir/backup_gzip");
rmtree("$tempdir/backup_gzip2");
+ rmtree("$tempdir/backup_gzip3");
}
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9e6807b457..1944647707 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -368,15 +368,24 @@ PostgreSQL documentation
<varlistentry>
<term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
+ <term><option>-Z [<replaceable class="parameter">method</replaceable></option>:[=<replaceable>level</replaceable>]]</term>
<term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
+ <term><option>--compress=[<replaceable class="parameter">method</replaceable></option>[:<replaceable>level</replaceable>]]</term>
<listitem>
<para>
- Enables gzip compression of tar file output, and specifies the
+ Enables compression of tar file output, and specifies the
compression level (0 through 9, 0 being no compression and 9 being best
compression). Compression is only available when using the tar
format, and the suffix <filename>.gz</filename> will
automatically be added to all tar filenames.
</para>
+ <para>
+ The compression method can be set to either <literal>gzip</literal>
+ for compression with <application>gzip</application>, or
+ <literal>none</literal> for no compression. A compression level
+ can be optionally specified, by appending the level number after a
+ colon (<literal>:</literal>).
+ </para>
</listitem>
</varlistentry>
</variablelist>
@@ -912,6 +921,16 @@ PostgreSQL documentation
<screen>
<prompt>$</prompt> <userinput>pg_basebackup -D backup/data -T /opt/ts=$(pwd)/backup/ts</userinput>
</screen></para>
+
+ <para>
+ To create a backup of a local server with one tar file for each tablespace
+ compressed with <application>gzip</application> at level 9, stored in the
+ directory <filename>backup</filename>:
+<screen>
+<prompt>$</prompt> <userinput>pg_basebackup -D backup -Ft --compress=gzip:9</userinput>
+</screen>
+ </para>
+
</refsect1>
<refsect1>
--
2.34.1
signature.asc
Description: PGP signature
