Re: pg_basebackup's --gzip switch misbehaves

Michael Paquier <michael(at)paquier(dot)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_basebackup/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 =
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 =
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