On Fri, Jan 14, 2022 at 04:53:12PM -0500, Robert Haas wrote: > On Thu, Jan 13, 2022 at 10:23 PM Michael Paquier <mich...@paquier.xyz> wrote: >> Using --compression-level=NUMBER and --server-compress=METHOD to >> specify a server-side compression method with a level is fine by me, >> but I find the reuse of --compress to specify a compression method >> confusing as it maps with the past option we have kept in >> pg_basebackup for a couple of years now. Based on your suggested set >> of options, we could then have a --client-compress=METHOD and >> --compression-level=NUMBER to specify a client-side compression method >> with a level. If we do that, I guess that we should then: >> 1) Block the combination of --server-compress and --client-compress. >> 2) Remove the existing -Z/--compress and -z/--gzip. > > I could live with that. I'm not sure that --client-compress instead of > reusing --compress is going to be better ... but I don't think it's > awful so much as just not my first choice. I also don't think it would > be horrid to leave -z, --gzip, and -Z as shorthands for the > --client-compress=gzip with --compression-level also in the last case, > instead of removing all that stuff.
Okay. So, based on this feedback, I guess that something like the attached would be what we are looking for. I have maximized the amount of code removed with the removal of -z/-Z, but I won't fight hard if the consensus is to keep them, either. We could also keep -z/--gzip, and stick -Z to the new --compression-level with --compress removed. -- Michael
From 1bbe624a4bea5e00a1997f81fe3b1ec1f637fa44 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Sat, 15 Jan 2022 11:51:40 +0900
Subject: [PATCH v4] Rework the option set of pg_basebackup
---
src/bin/pg_basebackup/pg_basebackup.c | 99 ++++++++++++++------
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 37 ++++----
doc/src/sgml/ref/pg_basebackup.sgml | 33 ++++---
3 files changed, 105 insertions(+), 64 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index aa43fc0924..57b054a8ad 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -113,6 +113,7 @@ static bool showprogress = false;
static bool estimatesize = true;
static int verbose = 0;
static int compresslevel = 0;
+static WalCompressionMethod client_method = COMPRESSION_NONE;
static IncludeWal includewal = STREAM_WAL;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
@@ -358,8 +359,10 @@ usage(void)
printf(_(" --waldir=WALDIR location for the write-ahead log directory\n"));
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(_(" --client-compress=METHOD\n"
+ " method to compress data (client-side)\n"));
+ printf(_(" --compression-level=1-9\n"
+ " compress tar output with given compression level\n"));
printf(_("\nGeneral options:\n"));
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
@@ -524,8 +527,7 @@ LogStreamerMain(logstreamer_param *param)
stream.do_sync);
else
stream.walmethod = CreateWalTarMethod(param->xlog,
- (compresslevel != 0) ?
- COMPRESSION_GZIP : COMPRESSION_NONE,
+ client_method,
compresslevel,
stream.do_sync);
@@ -976,7 +978,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;
@@ -1035,19 +1037,22 @@ CreateBackupStreamer(char *archive_name, char *spclocation,
archive_file = NULL;
}
+ if (client_method == COMPRESSION_NONE)
+ streamer = bbstreamer_plain_writer_new(archive_filename,
+ archive_file);
#ifdef HAVE_LIBZ
- if (compresslevel != 0)
+ else if (client_method == 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
@@ -1745,8 +1750,6 @@ main(int argc, char **argv)
{"slot", required_argument, NULL, 'S'},
{"tablespace-mapping", required_argument, NULL, 'T'},
{"wal-method", required_argument, NULL, 'X'},
- {"gzip", no_argument, NULL, 'z'},
- {"compress", required_argument, NULL, 'Z'},
{"label", required_argument, NULL, 'l'},
{"no-clean", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
@@ -1766,6 +1769,8 @@ main(int argc, char **argv)
{"no-manifest", no_argument, NULL, 5},
{"manifest-force-encode", no_argument, NULL, 6},
{"manifest-checksums", required_argument, NULL, 7},
+ {"client-compress", required_argument, NULL, 8},
+ {"compression-level", required_argument, NULL, 9},
{NULL, 0, NULL, 0}
};
int c;
@@ -1793,7 +1798,7 @@ main(int argc, char **argv)
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNzZ:d:c:h:p:U:s:wWkvP",
+ while ((c = getopt_long(argc, argv, "CD:F:r:RS:T:X:l:nNd:c:h:p:U:s:wWkvP",
long_options, &option_index)) != -1)
{
switch (c)
@@ -1872,18 +1877,6 @@ main(int argc, char **argv)
case 'N':
do_sync = false;
break;
- case 'z':
-#ifdef HAVE_LIBZ
- compresslevel = Z_DEFAULT_COMPRESSION;
-#else
- compresslevel = 1; /* will be rejected below */
-#endif
- break;
- case 'Z':
- if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
- &compresslevel))
- exit(1);
- break;
case 'c':
if (pg_strcasecmp(optarg, "fast") == 0)
fastcheckpoint = true;
@@ -1942,6 +1935,24 @@ main(int argc, char **argv)
case 7:
manifest_checksums = pg_strdup(optarg);
break;
+ case 8:
+ if (pg_strcasecmp(optarg, "gzip") == 0)
+ client_method = COMPRESSION_GZIP;
+ else if (pg_strcasecmp(optarg, "none") == 0)
+ client_method = COMPRESSION_NONE;
+ else
+ {
+ pg_log_error("invalid value \"%s\" for option %s",
+ optarg, "--client-compress");
+ exit(1);
+ }
+ break;
+ case 9:
+ if (!option_parse_int(optarg, "--compression-level", 1, 9,
+ &compresslevel))
+ exit(1);
+ break;
+ break;
default:
/*
@@ -1979,7 +1990,7 @@ main(int argc, char **argv)
/*
* Mutually exclusive arguments
*/
- if (format == 'p' && compresslevel != 0)
+ if (format == 'p' && client_method != COMPRESSION_NONE)
{
pg_log_error("only tar mode backups can be compressed");
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
@@ -2057,13 +2068,39 @@ main(int argc, char **argv)
}
}
-#ifndef HAVE_LIBZ
- if (compresslevel != 0)
+ /*
+ * Compression-related options.
+ */
+ switch (client_method)
{
- 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 --client-compress=%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 872fec3d35..0f84b3952a 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -31,6 +31,17 @@ 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", '--client-compress',
+ 'none', '--compression-level',
+ '1'
+ ],
+ qr/\Qpg_basebackup: error: cannot use --compression-level with --client-compress=none/,
+ 'failure if --compression-level specified with --client-compress=none');
+
# 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")
@@ -630,23 +641,14 @@ 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", 3
if (!check_pg_config("#define HAVE_LIBZ 1"));
$node->command_ok(
[
'pg_basebackup', '-D',
- "$tempdir/backup_gzip", '--compress',
- '1', '--no-sync',
- '--format', 't'
- ],
- 'pg_basebackup with --compress');
- $node->command_ok(
- [
- 'pg_basebackup', '-D',
- "$tempdir/backup_gzip2", '--gzip',
- '--no-sync', '--format',
- 't'
+ "$tempdir/backup_gzip", '--client-compress',
+ 'gzip', '--no-sync', '--format', 't'
],
'pg_basebackup with --gzip');
@@ -654,11 +656,8 @@ SKIP:
# 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)");
- 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)");
-
+ "two files created with --client-compress=gzip (base.tar.gz and pg_wal.tar.gz)"
+ );
# Check the integrity of the files generated.
my $gzip = $ENV{GZIP_PROGRAM};
skip "program gzip is not found in your system", 1
@@ -666,9 +665,7 @@ SKIP:
|| $gzip eq ''
|| system_log($gzip, '--version') != 0);
- my $gzip_is_valid =
- system_log($gzip, '--test', @zlib_files, @zlib_files2);
+ my $gzip_is_valid = system_log($gzip, '--test', @zlib_files);
is($gzip_is_valid, 0, "gzip verified the integrity of compressed data");
rmtree("$tempdir/backup_gzip");
- rmtree("$tempdir/backup_gzip2");
}
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9e6807b457..1a859794b2 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -354,28 +354,35 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
- <term><option>-z</option></term>
- <term><option>--gzip</option></term>
+ <term><option>--compression-level=<replaceable class="parameter">level</replaceable></option></term>
<listitem>
<para>
- Enables gzip compression of tar file output, with the default
- compression level. Compression is only available when using
- the tar format, and the suffix <filename>.gz</filename> will
- automatically be added to all tar filenames.
+ Controls the level of compression used with the tar file output,
+ and specifies the compression level (1 through 9, 1 being worst
+ 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>
+ This option is only available with <literal>--client-compress</literal>
+ set to <literal>gzip</literal>.
</para>
</listitem>
</varlistentry>
<varlistentry>
- <term><option>-Z <replaceable class="parameter">level</replaceable></option></term>
- <term><option>--compress=<replaceable class="parameter">level</replaceable></option></term>
+ <term><option>--client-compress=<replaceable class="parameter">method</replaceable></option></term>
<listitem>
<para>
- Enables gzip 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.
+ Enables client-side compression of backup data using the specified
+ method. Supported values are <literal>gzip</literal> and
+ <literal>none</literal>, the default.
+ </para>
+
+ <para>
+ The suffix <filename>.gz</filename> will automatically be added to
+ all filenames when using <literal>gzip</literal>.
</para>
</listitem>
</varlistentry>
--
2.34.1
signature.asc
Description: PGP signature
