Re: Introduce pg_receivewal gzip compression tests - Mailing list pgsql-hackers

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Tuesday, July 13th, 2021 at 03:53, Michael Paquier <michael@paquier.xyz> wrote:

> On Mon, Jul 12, 2021 at 04:46:29PM +0000, gkokolatos@pm.me wrote:
>
> > On Monday, July 12th, 2021 at 17:07, gkokolatos@pm.me wrote:
> >
> > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>
> Are you using outlook? The format of your messages gets blurry on the
>
> PG website, so does it for me.

I am using protonmail's web page. I was not aware of the issue. Thank you
for bringing it up to my attention. I shall try to address it.

>
> > > I am admittedly not so well versed on Windows systems. Thank you for
> > >
> > > informing me.
> > >
> > > Please find attached v3 of the patch where $ENV{GZIP_PROGRAM} is used
> > >
> > > instead. To the best of my knowledge one should avoid using $ENV{GZIP}
> > >
> > > because that would translate to the obsolete, yet used environment
> > >
> > > variable GZIP which holds a set of default options for gzip. In essence
> > >
> > > it would be equivalent to executing:
> > >
> > > GZIP=gzip gzip --test <files>
> > >
> > > which can result to errors similar to:
> > >
> > > gzip: gzip: non-option in GZIP environment variable
>
> -# make this available to TAP test scripts
>
> +# make these available to TAP test scripts
>
> export TAR
>
> +export GZIP_PROGRAM=$(GZIP)
>
> Wow. So this comes from the fact that the command gzip can feed on
>
> the environment variable from the same name. I was not aware of
>
> that, and a comment would be in place here. That means complicating a
>
> bit the test flow for people on Windows, but I am fine to live with
>
> that as long as this does not fail hard. One extra thing we could do
>
> is drop this part of the test, but I agree that this is useful to have
>
> around as a validity check.

Great.

>
> > After a bit more thinking, I went ahead and added on top of v3 a test
> >
> > verifying that the gzip program can actually be called.
>
> -         system_log($gzip, '--version') != 0);
>
>
>
> Checking after that does not hurt, indeed. I am wondering if we
>
> should do that for TAR.

I do not think that this will be a necessity for TAR. TAR after all
is discovered by autoconf, which gzip is not.

>
> Another thing I find unnecessary is the number of the tests. This
>
> does two rounds of pg_receivewal just to test the long and short
>
> options of -Z/-compress, which brings only coverage to make sure that
>
> both option names are handled. That's a high cost for a low amound of
>
> extra coverage, so let's cut the runtime in half and just use the
>
> round with --compress.

I am sorry this was not so clear. It is indeed running twice the binary
with different flags. However the goal is not to check the flags, but
to make certain that the partial file has now been completed. That is
why there was code asserting that the previous FILENAME.gz.partial file
after the second invocation is converted to FILENAME.gz.

Additionally the second invocation of pg_receivewal is extending the
coverage of FindStreamingStart().

The different flags was an added bonus.

>
> There was also a bit of confusion with ZLIB and gzip in the variable
>
> names and the comments, the latter being only the command while the
>
> compression happens with zlib. With a round of indentation on top of
>
> all that, I ge tthe attached.
>
> What do you think?

Thank you very much for the patch. I would prefer to keep the parts that
tests that .gz.partial are completed on a subsequent run if you agree.

Cheers,
//Georgios

>
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Michael