Thread: pgsql: Remove command checks in tests of pg_basebackup and pg_receivewa

Remove command checks in tests of pg_basebackup and pg_receivewal

The TAP tests of those commands have been checking if commands of "gzip"
and "lz4" existed by launching them with an extra --version.  Based on
the buildfarm, this is not required for "gzip" as the command always
exists.  Since 1d084fb, "lz4" has a ./configure check doing the same
thing.

Reported-by: Andres Freund
Discussion: https://postgr.es/m/20220212220643.ozuvq2k4cjkcnr2v@alap3.anarazel.de
Discussion: https://postgr.es/m/Ygm2ADakjlqGc2Ro@paquier.xyz

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/a4e1deb42b378650aaa2bcb3897caad9fe895957

Modified Files
--------------
src/bin/pg_basebackup/t/010_pg_basebackup.pl |  5 ++---
src/bin/pg_basebackup/t/020_pg_receivewal.pl | 10 ++++------
2 files changed, 6 insertions(+), 9 deletions(-)

On 2022-02-14 Mo 23:41, Michael Paquier wrote:
> Remove command checks in tests of pg_basebackup and pg_receivewal
>
> The TAP tests of those commands have been checking if commands of "gzip"
> and "lz4" existed by launching them with an extra --version.  Based on
> the buildfarm, this is not required for "gzip" as the command always
> exists.  Since 1d084fb, "lz4" has a ./configure check doing the same
> thing.
>
> Reported-by: Andres Freund
> Discussion: https://postgr.es/m/20220212220643.ozuvq2k4cjkcnr2v@alap3.anarazel.de
> Discussion: https://postgr.es/m/Ygm2ADakjlqGc2Ro@paquier.xyz
>


This appears to have been misconceived, at least in the case of MSVC
builds, as I have just discovered. It is entirely possible to have the
lz4 libraries installed and build with them but not have the .exe, and
unlike the configure case the MSVC build system doesn't conduct any test
for it, resulting in a nasty looking TAP test failure. I assume similar
failures are possible with zstd and maybe gzip.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, Apr 30, 2022 at 04:52:54PM -0400, Andrew Dunstan wrote:
> This appears to have been misconceived, at least in the case of MSVC
> builds, as I have just discovered. It is entirely possible to have the
> lz4 libraries installed and build with them but not have the .exe, and
> unlike the configure case the MSVC build system doesn't conduct any test
> for it, resulting in a nasty looking TAP test failure. I assume similar
> failures are possible with zstd and maybe gzip.

Okay, we could change the code of vcregress.pl so as the different ENV
commands (tar, zstd, lz4 or gzip) are assigned as follows for the sake
of the tests:
- If a ENV value is available, trust the environment/user and rely on
it.
- If a ENV value is not available, try to look for it in the
environment by launching a simple $command --version (-version should
be fine across all the commands currently in need of coverage?).
-- On failure, set ENV{command} to an empty string.
-- On success, set ENV{command} = "$command"

Does something like this look better to you?
--
Michael

On 2022-05-01 Su 09:15, Michael Paquier wrote:
> On Sat, Apr 30, 2022 at 04:52:54PM -0400, Andrew Dunstan wrote:
>> This appears to have been misconceived, at least in the case of MSVC
>> builds, as I have just discovered. It is entirely possible to have the
>> lz4 libraries installed and build with them but not have the .exe, and
>> unlike the configure case the MSVC build system doesn't conduct any test
>> for it, resulting in a nasty looking TAP test failure. I assume similar
>> failures are possible with zstd and maybe gzip.
> Okay, we could change the code of vcregress.pl so as the different ENV
> commands (tar, zstd, lz4 or gzip) are assigned as follows for the sake
> of the tests:
> - If a ENV value is available, trust the environment/user and rely on
> it.
> - If a ENV value is not available, try to look for it in the
> environment by launching a simple $command --version (-version should
> be fine across all the commands currently in need of coverage?).
> -- On failure, set ENV{command} to an empty string.
> -- On success, set ENV{command} = "$command"
>
> Does something like this look better to you?


IIRC we know that tar will be available on Windows.

I don't think we should do that check for every time we call
vc_regress.pl, that seems wasteful. Maybe do it if the command is one
that might require these commands, which I think would be bincheck or
taptest. And/Or stash some status somewhere?


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sun, May 01, 2022 at 10:18:37AM -0400, Andrew Dunstan wrote:
> IIRC we know that tar will be available on Windows.

What about gzip?  Are the binaries and the libraries split into
different packages for this package manager on Windows?  We've never
assumed that this is possible on ./configure, but it would make sense
to make MSVC able to manage this case.

> I don't think we should do that check for every time we call
> vc_regress.pl, that seems wasteful. Maybe do it if the command is one
> that might require these commands, which I think would be bincheck or
> taptest. And/Or stash some status somewhere?

Mostly.  We also care about gzip in contrib/basebackup_to_shell/,
though contribcheck does not run the TAP tests of contrib/ modules.
As you say, setting up those variables in bincheck and taptest would
be enough.
--
Michael

On 2022-05-02 Mo 03:06, Michael Paquier wrote:
> On Sun, May 01, 2022 at 10:18:37AM -0400, Andrew Dunstan wrote:
>> IIRC we know that tar will be available on Windows.
> What about gzip?  Are the binaries and the libraries split into
> different packages for this package manager on Windows?  We've never
> assumed that this is possible on ./configure, but it would make sense
> to make MSVC able to manage this case.


vcpkg doesn't install any.exe files AFAICS, just DLLs, headers, .lib
files etc.


>
>> I don't think we should do that check for every time we call
>> vc_regress.pl, that seems wasteful. Maybe do it if the command is one
>> that might require these commands, which I think would be bincheck or
>> taptest. And/Or stash some status somewhere?
> Mostly.  We also care about gzip in contrib/basebackup_to_shell/,
> though contribcheck does not run the TAP tests of contrib/ modules.
> As you say, setting up those variables in bincheck and taptest would
> be enough.


That would be a start. Maybe meson will us get rid of some of this mess ...


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Fri, May 06, 2022 at 10:12:36AM -0400, Andrew Dunstan wrote:
> On 2022-05-02 Mo 03:06, Michael Paquier wrote:
> vcpkg doesn't install any.exe files AFAICS, just DLLs, headers, .lib
> files etc.

Okay, thanks for confirming.

>>> I don't think we should do that check for every time we call
>>> vc_regress.pl, that seems wasteful. Maybe do it if the command is one
>>> that might require these commands, which I think would be bincheck or
>>> taptest. And/Or stash some status somewhere?
>>
>> Mostly.  We also care about gzip in contrib/basebackup_to_shell/,
>> though contribcheck does not run the TAP tests of contrib/ modules.
>> As you say, setting up those variables in bincheck and taptest would
>> be enough.
>
> That would be a start. Maybe meson will us get rid of some of this mess ...

Let's do that for now then as we need a middle ground for HEAD.  I'll
come up with something at the beginning of next week.
--
Michael

Hi Andrew,

On Sat, May 07, 2022 at 09:13:48AM +0900, Michael Paquier wrote:
> Let's do that for now then as we need a middle ground for HEAD.  I'll
> come up with something at the beginning of next week.

And here you go as of the attached to show the idea.  The CI is able
to execute and detect the default commands for lz4, gzip and zstd,
while one of my boxes without any of those commands would ignore them.
What do you think?
--
Michael

On 2022-05-09 Mo 01:28, Michael Paquier wrote:
> Hi Andrew,
>
> On Sat, May 07, 2022 at 09:13:48AM +0900, Michael Paquier wrote:
>> Let's do that for now then as we need a middle ground for HEAD.  I'll
>> come up with something at the beginning of next week.
> And here you go as of the attached to show the idea.  The CI is able
> to execute and detect the default commands for lz4, gzip and zstd,
> while one of my boxes without any of those commands would ignore them.
> What do you think?


The system() call should include redirection, since we only case about
the return code.


I would just write this


+    if ($rc != 0)
+    {
+        # Execution of the default command failed, so reset its
+        # environment value.
+        $ENV{$envname} = '';
+        return;
+    }
+
+    # Set the environment to the default as it exists.
+    $ENV{$envname} = $envdefault;
+    return;


as

    $ENV{$envname} = $envdefault if $rc == 0;

   

There should be no need to set it to the empty string in case if
failure, we already know it's undefined.


Otherwise looks good.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, May 09, 2022 at 10:10:27AM -0400, Andrew Dunstan wrote:
> The system() call should include redirection, since we only case about
> the return code.

Okay, done using File::Spec::devnull.

> I would just write this
>
> as
>
>     $ENV{$envname} = $envdefault if $rc == 0;
>
> There should be no need to set it to the empty string in case if
> failure, we already know it's undefined.

This needs to be adjusted on all the stable branches, so I'll do that
once this week's version is tagged.  Attached is an updated patch for
now.
--
Michael

On Tue, May 10, 2022 at 09:57:29AM +0900, Michael Paquier wrote:
> This needs to be adjusted on all the stable branches, so I'll do that
> once this week's version is tagged.  Attached is an updated patch for
> now.

The tags have been pushed, so done now.
--
Michael