Re: pg_basebackup's --gzip switch misbehaves

On Tue, Sep 13, 2022 at 05:38:47PM -0400, Tom Lane wrote:
> 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.

FWIW, I just stick a die() in the middle of the code path when I want
to look at specific results. Similar method, same result.

> # 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.

Another thing that could be done here is to use the same base location
as the cluster nodes aka $PostgreSQL::Test::Utils::tmp_check. That
would mean storing in a repo more data associated to the base backups
after a fresh run, though. I am not sure that small machine would
like this accumulation in a single run even if disk space is cheap
these days.

> 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.

I see more ways to change the existing behavior, so for now I have
left that untouched.

And so, I have spent a couple of hours torturing the patch, applying
it after a few tweaks and CI runs:
- --without-zlib was causing a failure in the pg_basebackup tests as
we have a few tests that parse and validate a set of invalid specs for
the client-side and server-side compression. With zlib around, the
tests and their expected results are unchanged, that's just a
consequence of moving the assignment of a default level much earlier.
- pg_basebackup was triggering an assertion when using client-lz4 or
client-zstd as we use the directory method of walmethods.c. In this
case, we just support zlib as compression and enforce no compression
when we are under lz4 or zstd. This came from an over-simplification
of the code. There is a gap in the testing of pg_basebackup,
actually, because we have zero tests for LZ4 and zstd there.
- The documentation of the replication protocol needed some
adjustments for the default comporession levels.

The buildfarm is green so I think that we are good. I have closed the
open item.

You have mentioned upthread an extra thing about the fact that we pass
down 0 to deflateParams(). This is indeed wrong and we are lucky that
Z_DEFAULT_STRATEGY maps to 0. Better to fix and backpatch this one
down to where gzip compression has been added to walmethods.c.. I'll
just do that in a bit after double-checking the area and the other
routines.
--
Michael