On Wed, Jan 19, 2022 at 08:35:23AM -0500, Robert Haas wrote: > I think that this will reject something like --compress=nonetheless by > telling you that 't' is not a valid separator. I think it would be > better to code this so that we first identify the portion preceding > the first colon, or the whole string if there is no colon. Then we > check whether that part is a compression method that we recognize. If > not, we complain.
Well, if no colon is specified, we still need to check if optarg is a pure integer if it does not match any of the supported methods, as --compress=0 should be backward compatible with no compression and --compress=1~9 should imply gzip, no? > If so, we then check whatever is after the separator > for validity - and this might differ by type. For example, we could > then immediately reject none:4, and if in the future we want to allow > lz4:fast3, we could. Okay. > I think the code that handles the bare integer case should be at the > top of the function and should return, because that code is short. > Then the rest of the function doesn't need to be indented as deeply. Done this way, I hope. -- Michael
From b5d84d8daac4c537657bfeef2ed0b5550106ffc2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 20 Jan 2022 15:59:54 +0900
Subject: [PATCH v6] Rework the option set of pg_basebackup
---
src/bin/pg_basebackup/pg_basebackup.c | 150 ++++++++++++++++---
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 46 +++++-
doc/src/sgml/ref/pg_basebackup.sgml | 21 ++-
3 files changed, 194 insertions(+), 23 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2a58be638a..ae003766cc 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,84 @@ 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)
+{
+ char *sep;
+ int firstlen;
+ char *firstpart = NULL;
+
+ /* check if the option is split in two */
+ sep = strchr(src, ':');
+
+ /*
+ * The first part of the option value could be a method name, or just
+ * a level value.
+ */
+ firstlen = (sep != NULL) ? (sep - src) : strlen(src);
+ firstpart = pg_malloc(firstlen + 1);
+ strncpy(firstpart, src, firstlen);
+ firstpart[firstlen] = '\0';
+
+ /*
+ * Check if the first part of the string matches with a supported
+ * compression method.
+ */
+ if (pg_strcasecmp(firstpart, "gzip") != 0 &&
+ pg_strcasecmp(firstpart, "none") != 0)
+ {
+ /*
+ * It does not match anything known, so check for the
+ * backward-compatible case of only an integer, where the implied
+ * compression method changes depending on the level value.
+ */
+ if (!option_parse_int(firstpart, "-Z/--compress", 0,
+ INT_MAX, levelres))
+ exit(1);
+
+ *methodres = (*levelres > 0) ?
+ COMPRESSION_GZIP : COMPRESSION_NONE;
+ return;
+ }
+
+ /* Supported method found. */
+ if (pg_strcasecmp(firstpart, "gzip") == 0)
+ *methodres = COMPRESSION_GZIP;
+ else if (pg_strcasecmp(firstpart, "none") == 0)
+ *methodres = COMPRESSION_NONE;
+
+ if (sep == NULL)
+ {
+ /*
+ * The caller specified a method without a colon separator, so let
+ * any subsequent checks assign a default.
+ */
+ return;
+ }
+
+ /* Check the contents after the colon separator. */
+ sep++;
+ if (*sep == '\0')
+ {
+ pg_log_error("no compression level defined for method %s", firstpart);
+ exit(1);
+ }
+
+ /*
+ * 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,
+ levelres))
+ exit(1);
+}
+
/*
* Read a stream of COPY data and invoke the provided callback for each
* chunk.
@@ -993,7 +1072,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 +1131,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 +2302,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 +2403,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 +2481,45 @@ 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;
+ }
+ if (compresslevel > 9)
+ {
+ pg_log_error("compression level %d of method %s higher than maximum of 9",
+ compresslevel, "gzip");
+ exit(1);
+ }
+#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..2621296a49 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 value "none+" for option/,
+ 'failure on incorrect separator to define compression level');
+$node->command_fails_like(
+ [
+ 'pg_basebackup', '-D',
+ "$tempdir/backup", '--compress',
+ 'none:'
+ ],
+ qr/\Qpg_basebackup: error: no compression level defined for method none/,
+ '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..feabd637eb 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
