Subject: Re: Refactoring of compression options in pg_basebackup

From: [email-masked]
Date: Wed, 05 Jan 2022 09:17:28 +0000

On Wednesday, January 5th, 2022 at 9:00 AM, Michael Paquier <[email-masked]> wrote:

> On Mon, Jan 03, 2022 at 03:35:57PM +0000, [email-masked] wrote:
> > I propose to initialize streamer to NULL when declared at the top of
> > CreateBackupStreamer().
>
> Yes, that may be noisy.  Done this way.

Great.

> > You can see that after the check_pg_config() test, only 3 tests follow,
> > namely:
> >  *  $node->command_ok()
> >  *  is(scalar(), ...)
> >  *  is($gzip_is_valid, ...)
>
> Indeed.  The CF bot was complaining about that, actually.

Great.

> Thinking more about this stuff, pg_basebackup --compress is an option
> that exists already for a couple of years, and that's independent of
> the backend-side compression that Robert and Jeevan are working on, so
> I'd like to move on this code cleanup.  We can always figure out the
> LZ4 part for pg_basebackup after, if necessary.

I agree that the cleanup in itself is helpful. It feels awkward to have two
utilities under the same path, with distinct options for the same
functionality.

When the backend-side compression is completed, were there really be a need for
client-side compression? If yes, then it seems logical to have distinct options
for them and this cleanup makes sense. If not, then it seems logical to maintain
the current options list and 'simply' change the internals of the code, and this
cleanup makes sense.

> Attached is an updated patch.  The CF bot should improve with that.

+1

> --
> Michael

Cheers,
//Georgios

---

From: Robert Haas <[email-masked]>
Date: Wed, 5 Jan 2022 10:33:38 -0500

> the current options list and 'simply' change the internals of the code, and this

I think we're going to want to offer both options. We can't know
whether the user prefers to consume CPU cycles on the server or on the
client. Compressing on the server has the advantage of potentially
saving transfer bandwidth, but the server is also often the busiest
part of the whole system, and users are often keen to offload as much
work as possible.

Given that, I'd like us to be thinking about what the full set of
options looks like once we have (1) compression on either the server
or the client and (2) multiple compression algorithms and (3) multiple
compression levels. Personally, I don't really like the decision made
by this proposed patch. In this patch's view of the world, -Z is a way
of providing the compression level for whatever compression algorithm
you happen to have selected, but I think of -Z as being the upper-case
version of -z which I think of as selecting specifically gzip. It's
not particularly intuitive to me that in a command like pg_basebackup
--compress=<something>, <something> is a compression level rather than
an algorithm. So what I would propose is probably something like:

pg_basebackup --compress=ALGORITHM [--compression-level=NUMBER]
pg_basebackup --server-compress=ALGORITHM [--compression-level=NUMBER]

And then make -z short for --compress=gzip and -Z <n> short for
--compress=gzip --compression-level=<n>. That would be a
backward-incompatible change to the definition of --compress, but as
long as -Z <n> works the same as today, I don't think many people will
notice. If we like, we can notice if the argument to --compress is an
integer and suggest using either -Z or --compress=gzip
--compression-level=<n> instead.

In the proposed patch, you end up with pg_basebackup
--compression-method=lz4 -Z2 meaning compression with lz4 level 2. I
find that quite odd, though as with all such things, opinions may
vary. In my proposal, that would be an error, because it would be
equivalent to --compress=lz4 --compress=gzip --compression-level=2,
and would thus involve conflicting compression method specifications.

--
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Michael Paquier <[email-masked]>
Date: Thu, 6 Jan 2022 14:04:07 +0900

> part of the whole system, and users are often keen to offload as much

Yeah.  There are cases for both.  I just got to wonder whether it
makes sense to allow both server-side and client-side compression to
be used at the same time.  That would be a rather strange case, but
well, with the correct set of options that could be possible.

> --compression-level=<n> instead.

My view of things is slightly different, aka I'd rather keep
--compress to mean a compression level with an integer option, but
introduce a --compression-method={lz4,gzip,none}, with -Z being a
synonym of --compression-method=gzip.  That's at least the path we
chose for pg_receivewal.  I don't mind sticking with one way or
another, as what you are proposing is basically the same thing I have
in mind, but both tools ought to use the same set of options.

Hmm.  Perhaps at the end the problem is with --compress, where we
don't know if it means a compression level or a compression method?
For me, --compress means the former, and for you the latter.  So a
third way of seeing things is to drop completely --compress, but have
one --compression-method and one --compression-level.  That would
bring a clear split.  Or just one --compression-method for the
client-side compression as you are proposing for the server-side
compression, however I'd like to think that a split between the method
and level is more intuitive.

> and would thus involve conflicting compression method specifications.

It seems to me that you did not read the patch closely enough.  The
attached patch does not add support for LZ4 in pg_basebackup on the
client-side yet.  Once it gets added, though, the idea is that  using
--compress with LZ4 would result in an error.  That's what happens
with pg_receivewal on HEAD, for one.  The patch just shapes things to
plug LZ4 more easily in the existing code of pg_basebackup.c, and
walmethods.c.

So..  As of now, it is actually possible to cut the pie in three
parts.  There are no real objections to the cleanup of walmethods.c
and the addition of some conditional TAP tests with pg_basebackup and 
client-side compression, as far as I can see, only to the option
renaming part.  Attached are two patches, then.  0001 is the cleanup
of walmethods.c to rely the compression method, with more tests (tests
that could be moved into their own patch, as well).  0002 is the
addition of the options I suggested upthread, but we may change that 
depending on what gets used for the server-side compression for
consistency so I am not suggesting to merge that until we agree on the
full picture.  The point of this thread was mostly about 0001, so I am
fine to discard 0002.  Thoughts?
--
Michael
From b2beb69c23ea8346a684e8c7ae5a5b60bade2066 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 6 Jan 2022 13:46:56 +0900
Subject: [PATCH v3 1/2] Refactor walmethods.c's tar method to use compression
 method

Some TAP tests are added for pg_basebackup --compress, while on it.
---
 src/bin/pg_basebackup/pg_basebackup.c        |  3 +-
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 36 ++++++++++++++-
 src/bin/pg_basebackup/walmethods.c           | 47 +++++++++++++-------
 3 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 1739ac6382..17ff0132d9 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -524,7 +524,8 @@ LogStreamerMain(logstreamer_param *param)
 													stream.do_sync);
 	else
 		stream.walmethod = CreateWalTarMethod(param->xlog,
-											  COMPRESSION_NONE, /* ignored */
+											  (compresslevel > 0) ?
+											  COMPRESSION_GZIP : COMPRESSION_NONE,
 											  compresslevel,
 											  stream.do_sync);
 
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index e56825382c..192781e0a2 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 => 110;
+use Test::More tests => 113;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -624,3 +624,37 @@ rmtree("$tempdir/backup_corrupt4");
 
 $node->safe_psql('postgres', "DROP TABLE corrupt1;");
 $node->safe_psql('postgres', "DROP TABLE corrupt2;");
+
+note "Testing pg_basebackup with compression methods";
+
+# Check ZLIB compression if available.
+SKIP:
+{
+	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');
+
+	# 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 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
+	  if ( !defined $gzip
+		|| $gzip eq ''
+		|| system_log($gzip, '--version') != 0);
+
+	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");
+}
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.c
index affdc5055f..127bb5e013 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -749,7 +749,7 @@ tar_write(Walfile f, const void *buf, size_t count)
 	tar_clear_error();
 
 	/* Tarfile will always be positioned at the end */
-	if (!tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_NONE)
 	{
 		errno = 0;
 		r = write(tar_data->fd, buf, count);
@@ -833,7 +833,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		}
 
 #ifdef HAVE_LIBZ
-		if (tar_data->compression_level)
+		if (tar_data->compression_method == COMPRESSION_GZIP)
 		{
 			tar_data->zp = (z_streamp) pg_malloc(sizeof(z_stream));
 			tar_data->zp->zalloc = Z_NULL;
@@ -884,7 +884,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	pg_free(tmppath);
 
 #ifdef HAVE_LIBZ
-	if (tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_GZIP)
 	{
 		/* Flush existing data */
 		if (!tar_write_compressed_data(NULL, 0, true))
@@ -909,7 +909,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	}
 	tar_data->currentfile->currpos = 0;
 
-	if (!tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_NONE)
 	{
 		errno = 0;
 		if (write(tar_data->fd, tar_data->currentfile->header,
@@ -923,7 +923,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		}
 	}
 #ifdef HAVE_LIBZ
-	else
+	else if (tar_data->compression_method == COMPRESSION_GZIP)
 	{
 		/* Write header through the zlib APIs but with no compression */
 		if (!tar_write_compressed_data(tar_data->currentfile->header,
@@ -938,6 +938,10 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 		}
 	}
 #endif
+	else
+	{
+		Assert(false);			/* not reachable */
+	}
 
 	tar_data->currentfile->pathname = pg_strdup(pathname);
 
@@ -948,7 +952,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
 	if (pad_to_size)
 	{
 		tar_data->currentfile->pad_to_size = pad_to_size;
-		if (!tar_data->compression_level)
+		if (tar_data->compression_method == COMPRESSION_NONE)
 		{
 			/* Uncompressed, so pad now */
 			if (!tar_write_padding_data(tar_data->currentfile, pad_to_size))
@@ -1009,7 +1013,7 @@ tar_sync(Walfile f)
 	 * Always sync the whole tarfile, because that's all we can do. This makes
 	 * no sense on compressed files, so just ignore those.
 	 */
-	if (tar_data->compression_level)
+	if (tar_data->compression_method != COMPRESSION_NONE)
 		return 0;
 
 	r = fsync(tar_data->fd);
@@ -1030,7 +1034,7 @@ tar_close(Walfile f, WalCloseMethod method)
 
 	if (method == CLOSE_UNLINK)
 	{
-		if (tar_data->compression_level)
+		if (tar_data->compression_method != COMPRESSION_NONE)
 		{
 			tar_set_error("unlink not supported with compression");
 			return -1;
@@ -1061,7 +1065,7 @@ tar_close(Walfile f, WalCloseMethod method)
 	 */
 	if (tf->pad_to_size)
 	{
-		if (tar_data->compression_level)
+		if (tar_data->compression_method == COMPRESSION_GZIP)
 		{
 			/*
 			 * A compressed tarfile is padded on close since we cannot know
@@ -1102,7 +1106,7 @@ tar_close(Walfile f, WalCloseMethod method)

 #ifdef HAVE_LIBZ
-	if (tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_GZIP)
 	{
 		/* Flush the current buffer */
 		if (!tar_write_compressed_data(NULL, 0, true))
@@ -1131,7 +1135,7 @@ tar_close(Walfile f, WalCloseMethod method)
 		tar_data->lasterrno = errno;
 		return -1;
 	}
-	if (!tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_NONE)
 	{
 		errno = 0;
 		if (write(tar_data->fd, tf->header, TAR_BLOCK_SIZE) != TAR_BLOCK_SIZE)
@@ -1142,7 +1146,7 @@ tar_close(Walfile f, WalCloseMethod method)
 		}
 	}
 #ifdef HAVE_LIBZ
-	else
+	else if (tar_data->compression_method == COMPRESSION_GZIP)
 	{
 		/* Turn off compression */
 		if (deflateParams(tar_data->zp, 0, 0) != Z_OK)
@@ -1164,6 +1168,10 @@ tar_close(Walfile f, WalCloseMethod method)
 		}
 	}
 #endif
+	else
+	{
+		Assert(false);			/* not reachable */
+	}
 
 	/* Move file pointer back down to end, so we can write the next file */
 	if (lseek(tar_data->fd, 0, SEEK_END) < 0)
@@ -1212,7 +1220,7 @@ tar_finish(void)
 
 	/* A tarfile always ends with two empty blocks */
 	MemSet(zerobuf, 0, sizeof(zerobuf));
-	if (!tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_NONE)
 	{
 		errno = 0;
 		if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
@@ -1223,7 +1231,7 @@ tar_finish(void)
 		}
 	}
 #ifdef HAVE_LIBZ
-	else
+	else if (tar_data->compression_method == COMPRESSION_GZIP)
 	{
 		if (!tar_write_compressed_data(zerobuf, sizeof(zerobuf), false))
 			return false;
@@ -1268,6 +1276,10 @@ tar_finish(void)
 		}
 	}
 #endif
+	else
+	{
+		Assert(false);			/* not reachable */
+	}
 
 	/* sync the empty blocks as well, since they're after the last file */
 	if (tar_data->sync)
@@ -1312,7 +1324,8 @@ CreateWalTarMethod(const char *tarbase,
 				   int compression_level, bool sync)
 {
 	WalWriteMethod *method;
-	const char *suffix = (compression_level != 0) ? ".tar.gz" : ".tar";
+	const char *suffix = (compression_method == COMPRESSION_GZIP) ?
+	".tar.gz" : ".tar";
 
 	method = pg_malloc0(sizeof(WalWriteMethod));
 	method->open_for_write = tar_open_for_write;
@@ -1335,7 +1348,7 @@ CreateWalTarMethod(const char *tarbase,
 	tar_data->compression_level = compression_level;
 	tar_data->sync = sync;
 #ifdef HAVE_LIBZ
-	if (compression_level)
+	if (compression_method == COMPRESSION_GZIP)
 		tar_data->zlibOut = (char *) pg_malloc(ZLIB_OUT_SIZE + 1);
 #endif
 
@@ -1347,7 +1360,7 @@ FreeWalTarMethod(void)
 {
 	pg_free(tar_data->tarfilename);
 #ifdef HAVE_LIBZ
-	if (tar_data->compression_level)
+	if (tar_data->compression_method == COMPRESSION_GZIP)
 		pg_free(tar_data->zlibOut);
 #endif
 	pg_free(tar_data);
-- 
2.34.1

From c6518d28cbc9e18f5445163a756692333529519d Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Thu, 6 Jan 2022 13:53:46 +0900
Subject: [PATCH v3 2/2] Rework the option set of pg_basebackup

---
 src/bin/pg_basebackup/pg_basebackup.c        | 82 +++++++++++++++-----
 src/bin/pg_basebackup/t/010_pg_basebackup.pl | 19 ++++-
 doc/src/sgml/ref/pg_basebackup.sgml          | 22 +++++-
 3 files changed, 95 insertions(+), 28 deletions(-)

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 17ff0132d9..7f29200832 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 compression_method = COMPRESSION_NONE;
 static IncludeWal includewal = STREAM_WAL;
 static bool fastcheckpoint = false;
 static bool writerecoveryconf = false;
@@ -359,7 +360,9 @@ 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=1-9     compress tar output with given compression level\n"));
+	printf(_("      --compression-method=METHOD\n"
+			 "                         method to compress data\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,
+											  compression_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 (compression_method == COMPRESSION_NONE)
+			streamer = bbstreamer_plain_writer_new(archive_filename,
+												   archive_file);
 #ifdef HAVE_LIBZ
-		if (compresslevel != 0)
+		else if (compression_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
@@ -1766,6 +1771,7 @@ main(int argc, char **argv)
 		{"no-manifest", no_argument, NULL, 5},
 		{"manifest-force-encode", no_argument, NULL, 6},
 		{"manifest-checksums", required_argument, NULL, 7},
+		{"compression-method", required_argument, NULL, 8},
 		{NULL, 0, NULL, 0}
 	};
 	int			c;
@@ -1873,14 +1879,10 @@ main(int argc, char **argv)
 				do_sync = false;
 				break;
 			case 'z':
-#ifdef HAVE_LIBZ
-				compresslevel = Z_DEFAULT_COMPRESSION;
-#else
-				compresslevel = 1;	/* will be rejected below */
-#endif
+				compression_method = COMPRESSION_GZIP;
 				break;
 			case 'Z':
-				if (!option_parse_int(optarg, "-Z/--compress", 0, 9,
+				if (!option_parse_int(optarg, "-Z/--compress", 1, 9,
 									  &compresslevel))
 					exit(1);
 				break;
@@ -1942,6 +1944,18 @@ main(int argc, char **argv)
 			case 7:
 				manifest_checksums = pg_strdup(optarg);
 				break;
+			case 8:
+				if (pg_strcasecmp(optarg, "gzip") == 0)
+					compression_method = COMPRESSION_GZIP;
+				else if (pg_strcasecmp(optarg, "none") == 0)
+					compression_method = COMPRESSION_NONE;
+				else
+				{
+					pg_log_error("invalid value \"%s\" for option %s",
+								 optarg, "--compression-method");
+					exit(1);
+				}
+				break;
 			default:
 
 				/*
@@ -1979,7 +1993,7 @@ main(int argc, char **argv)
 	/*
 	 * Mutually exclusive arguments
 	 */
-	if (format == 'p' && compresslevel != 0)
+	if (format == 'p' && compression_method != COMPRESSION_NONE)
 	{
 		pg_log_error("only tar mode backups can be compressed");
 		fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
@@ -2057,13 +2071,39 @@ main(int argc, char **argv)
 		}
 	}
 
-#ifndef HAVE_LIBZ
-	if (compresslevel != 0)
+	/*
+	 * Compression-related options.
+	 */
+	switch (compression_method)
 	{
-		pg_log_error("this build does not support compression");
-		exit(1);
-	}
+		case COMPRESSION_NONE:
+			if (compresslevel != 0)
+			{
+				pg_log_error("cannot use --compress with --compression-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 --compress, 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 192781e0a2..e5fec5f813 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 => 113;
+use Test::More tests => 115;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -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", '--compression-method',
+		'none',            '--compress',
+		'1'
+	],
+	qr/\Qpg_basebackup: error: cannot use --compress with --compression-method=none/,
+	'failure if --compress specified with --compression-method=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")
@@ -636,10 +647,10 @@ SKIP:
 	$node->command_ok(
 		[
 			'pg_basebackup',        '-D',
-			"$tempdir/backup_gzip", '--compress', '1',
-			'--no-sync', '--format', 't'
+			"$tempdir/backup_gzip", '--compression-method',
+			'gzip', '--compress', '1', '--no-sync', '--format', 't'
 		],
-		'pg_basebackup with --compress');
+		'pg_basebackup with --compress and --compression-method=gzip');
 
 	# Verify that the stored files are generated with their expected
 	# names.
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9e6807b457..9cbec844c8 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -372,9 +372,9 @@ PostgreSQL documentation
       <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
+        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>
       </listitem>
@@ -567,6 +567,22 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>--compression-method=<replaceable class="parameter">method</replaceable></option></term>
+      <listitem>
+       <para>
+        Enables compression of backup data using the specified method.
+        Supported values <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>
+     
      <varlistentry>
       <term><option>--manifest-force-encode</option></term>
       <listitem>
-- 
2.34.1

---

From: Robert Haas <[email-masked]>
Date: Thu, 6 Jan 2022 09:27:19 -0500

> well, with the correct set of options that could be possible.

I don't think it makes sense to support that. On the one hand, it
doesn't seem useful: compressing already-compressed data doesn't
usually work very well. Alternatively, I suppose the intent could be
to compress one way for transfer and then decompress and recompress
for storage, but that seems too inefficient to take seriously. On the
other hand, it requires a more complex user interface, and it's
already fairly complicated anyway.

> in mind, but both tools ought to use the same set of options.

Did you mean that -z would be a synonym for --compression-method=gzip?
It doesn't really make sense for -Z to be that, unless it's also
setting the compression level.

My objection to --compress=$LEVEL is that the compression level seems
like it ought to rightfully be subordinate to the choice of algorithm.
In general, there's no reason why a compression algorithm has to offer
a choice of compression levels at all, or why they have to be numbered
0 through 9. For example, lz4 on my system claims to offer compression
levels from 1 through 12, plus a separate set of "fast" compression
levels starting with 1 and going up to an unspecified number. And then
it also has options to favor decompression speed, change the block
size, and a few other parameters. We don't necessarily want to expose
all of those options, but we should structure things so that we could
if it became important. The way to do that is to make the compression
algorithm the primary setting, and then anything else you can set for
that compressor is somehow a subordinate setting.

Put another way, we don't decide first that we want to compress with
level 7, and then afterward decide whether that's gzip, lz4, or bzip2.
We pick the compressor first, and then MAYBE think about changing the
compression level.

> plug LZ4 more easily in the existing code of pg_basebackup.c, and

Well what I was looking at was this:

- printf(_("  -Z, --compress=0-9     compress tar output with given
compression level\n"));
+ printf(_("  -Z, --compress=1-9     compress tar output with given
compression level\n"));
+ printf(_("      --compression-method=METHOD\n"
+ "                         method to compress data\n"));

That seems to show that, post-patch, the argument to -Z would be a
compression level, even if --compression-method were something other
than gzip.

It's possible that I haven't read something carefully enough, but to
me, what I said seems to be a straightforward conclusion based on
looking at the usage help in the patch. So if I came to the wrong
conclusion, perhaps that usage help isn't reflecting the situation you
intend to create, or not as clearly as it ought.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 7 Jan 2022 15:43:06 +0900

> setting the compression level.

Yes, I meant "-z", not "-Z", to be a synonym of
--compression-method=gzip.  Sorry for the typo.

> that compressor is somehow a subordinate setting.

For any compression method, that maps to an integer, so..  But I am
not going to fight hard on that.

> We pick the compressor first, and then MAYBE think about changing the

Which is why things should be checked once all the options are
processed.  I'd recommend that you read the option patch a bit more,
that may help.

> compression level, even if --compression-method were something other

Yes, after the patch --compress would be a compression level.  And, if
attempting to use with --compression-method set to "none", or
potentially "lz4", it would just fail.  If not using this "gzip", the
compression level is switched to Z_DEFAULT_COMPRESSION.  That's this
area of the patch, FWIW:
+   /*
+    * Compression-related options.
+    */
+   switch (compression_method)

> intend to create, or not as clearly as it ought.

Perhaps the --help output could be clearer, then.  Do you have a
suggestion?

Bringing walmethods.c at the same page for the directory and the tar
methods was my primary goal here, and the tests are a bonus, so I've
applied this part for now, leaving pg_basebackup alone until we figure
out the layer of options we should use.  Perhaps it would be better to
revisit that stuff once the server-side compression has landed.
--
Michael

---

From: Robert Haas <[email-masked]>
Date: Thu, 13 Jan 2022 13:36:00 -0500

> processed.  I'd recommend that you read the option patch a bit more,

I don't think that the problem here is my lack of understanding. I
have two basic concerns about your proposed patch:

1. If, as you propose, we add a new flag --compression-method=METHOD
then how will the user specify server-side compression?
2. If, as we seem to agree, the compression method is more important
than the compression level, then why is the option to set the
less-important thing called just --compress, and the option to set the
more important thing has a longer name?

I proposed to solve both of these problems by using
--compression-level=NUMBER to set the compression level and
--compress=METHOD or --server-compress=METHOD to set the algorithm and
specify on which side it is to be applied. If, instead of doing that,
we go with what you have proposed here, then I don't understand how to
fit server-side compression into the framework in a reasonably concise
way. I think we would end up with something like pg_basebackup
--compression-method=lz4 --compress-on-server, which seems rather long
and awkward. Do you have a better idea?

I think I understand what the patch is doing. I just think it creates
a problem for my patch. And I'd like to know whether you have an idea
how to solve that problem. And if not, then I'd like you to consider
the solution that I am proposing rather than the patch you've already
got.

---

From: Michael Paquier <[email-masked]>
Date: Fri, 14 Jan 2022 12:23:27 +0900

> then how will the user specify server-side compression?

This would require a completely different option switch, which is
basically the same thing as what you are suggesting with
--server-compress.

> more important thing has a longer name?

I agree that the method is more important than the level for most
users, and I would not mind dropping completely --compress in favor of
something else, which is something I implied upthread.

> and awkward. Do you have a better idea?

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.

You have implied 1) upthread as far as I recall, 2) is something I am
adding on top of it.

> the solution that I am proposing rather than the patch you've already

I am fine to drop this thread's patch with its set of options and work
on top of your proposal, aka what's drafted two paragraphs above.
--
Michael