From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pg_basebackup's --gzip switch misbehaves
In-reply-to: <YyAtkOU/C/SD9qlL@paquier.xyz>
References: <1400032.1662217889@sss.pgh.pa.us> <YxQ1tIYF8hbrNO7q@paquier.xyz>
 <YyAtkOU/C/SD9qlL@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Tue, 13 Sep 2022 16:13:20 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2402846.1663105127.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 13 Sep 2022 17:38:47 -0400
Message-ID: <2402847.1663105127@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2402847.1663105127%40sss.pgh.pa.us>
Precedence: bulk

Michael Paquier <michael@paquier.xyz> writes:
> Attached is the patch I am finishing with, consisting of:
> - the removal of PG_COMPRESSION_OPTION_LEVEL.
> - assigning a default compression level when nothing is specified in
> the spec.
> - a couple of complifications in pg_receivewal, pg_basebackup and the
> backend code as there is no need to worry about the compression
> level.

This looks good to me.  It seems simpler, and I concur that it
fixes the described problem.  I now see

tmp_check/backup_gzip:
total 3668
-rw-r-----. 1 postgres postgres  137756 Sep 13 17:29 backup_manifest
-rw-r-----. 1 postgres postgres 3537499 Sep 13 17:29 base.tar.gz
-rw-r-----. 1 postgres postgres   73989 Sep 13 17:29 pg_wal.tar.gz

tmp_check/backup_gzip2:
total 3168
-rw-r-----. 1 postgres postgres  137756 Sep 13 17:29 backup_manifest
-rw-r-----. 1 postgres postgres 3083516 Sep 13 17:29 base.tar.gz
-rw-r-----. 1 postgres postgres   17069 Sep 13 17:29 pg_wal.tar.gz

tmp_check/backup_gzip3:
total 3668
-rw-r-----. 1 postgres postgres  137756 Sep 13 17:29 backup_manifest
-rw-r-----. 1 postgres postgres 3537517 Sep 13 17:29 base.tar.gz
-rw-r-----. 1 postgres postgres   73988 Sep 13 17:29 pg_wal.tar.gz

which looks sane: the gzip2 case should, and does, have better
compression than the other two.

BTW, this bit:

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_bas=
ebackup/t/010_pg_basebackup.pl
index 3d1a4ddd5c..40f1d3f7e2 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -860,9 +860,6 @@ SKIP:
 	my $gzip_is_valid =3D
 	  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");
 }
 =

 # Test background stream process terminating before the basebackup has

is something I tried along the way to diagnosing the problem, and
it turns out to have exactly zero effect.  The $tempdir is some
temporary subdirectory of tmp_check that will get nuked at the end
of the TAP test no matter what.  So these rmtrees are merely making
the evidence disappear a bit faster; it will anyway.

What I did to diagnose the problem was this:

@@ -860,9 +860,9 @@ SKIP:
    my $gzip_is_valid =3D
      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");
+   system_log('mv', "$tempdir/backup_gzip", "tmp_check");
+   system_log('mv', "$tempdir/backup_gzip2", "tmp_check");
+   system_log('mv', "$tempdir/backup_gzip3", "tmp_check");
 }
 =

 # Test background stream process terminating before the basebackup has

which is not real clean, since then the files get left behind even
on success, which I doubt we want either.

Anyway, I have no objection to dropping the rmtrees, since they're
pretty useless as the code stands.  Just wanted to mention this
issue for the archives.

			regards, tom lane