pg_basebackup: add test about zstd compress option

On Tue, Aug 23, 2022 at 10:58:22AM +0900, Dong Wook Lee wrote:

I checked the test code not to test the zstd option, then added it.
I hope my patch will help us to ensure safety of the test.

It seems to me that checking that the contents generated are valid is
equally necessary. We do that with zlib with gzip --test, and you
could use ${ZSTD} in the context of this test.

What about lz4?
--
Michael

On Tue, Aug 23, 2022 at 11:37 AM Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that checking that the contents generated are valid is
equally necessary. We do that with zlib with gzip --test, and you
could use ${ZSTD} in the context of this test.

Thank you for the good points.
I supplemented the test according to your suggestion.
However, there was a problem.
Even though I did export ZSTD on the Makefile , the test runner can't
find ZSTD when it actually tests.
```
my $zstd = $ENV{ZSTD};
skip "program zstd is not found in your system", 1
if (!defined $zstd
|| $zstd eq '');
```
log: regress_log_010_pg_basebackup
```
ok 183 # skip program zstd is not found in your system.
```
Could you check if I missed anything?

On Thu, Aug 25, 2022 at 3:52 AM Dong Wook Lee <sh95119@gmail.com> wrote:

Could you check if I missed anything?

There is already a far better test for this in
src/bin/pg_verifybackup/t/009_extract.pl

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi

I was looking at the commitfest entry for this patch [1]https://commitfest.postgresql.org/40/3835/ as it's been dormant
for quite a while, with the intent of returning it with feedback.

[1]: https://commitfest.postgresql.org/40/3835/

2022年8月25日(木) 16:52 Dong Wook Lee <sh95119@gmail.com>:

On Tue, Aug 23, 2022 at 11:37 AM Michael Paquier <michael@paquier.xyz> wrote:

It seems to me that checking that the contents generated are valid is
equally necessary. We do that with zlib with gzip --test, and you
could use ${ZSTD} in the context of this test.

Thank you for the good points.
I supplemented the test according to your suggestion.
However, there was a problem.
Even though I did export ZSTD on the Makefile , the test runner can't
find ZSTD when it actually tests.
```
my $zstd = $ENV{ZSTD};
skip "program zstd is not found in your system", 1
if (!defined $zstd
|| $zstd eq '');
```
log: regress_log_010_pg_basebackup
```
ok 183 # skip program zstd is not found in your system.
```
Could you check if I missed anything?

Taking a quick look at the patch itself, as-is it does actually work; maybe
the zstd binary itself was missing or not in the normal system path?
It might not have been installed even if the devel library was (IIRC
this was the case on Rocky Linux).

However the code largely duplicates the preceding gzip test,
and as Michael mentioned there's still lz4 without coverage.
Attached patch refactors this part of the test so it can be used
for multiple compression methods, similar to the test in
src/bin/pg_verifybackup/t/009_extract.pl mentioned by Robert.
The difference to that test is that we can exercise all the
command line options and directly check the generated files with
the respective binary.

Though on reflection maybe it's overkill and the existing tests
suffice. Anyway leaving the patch here in the interests of pushing
this forward in some direction.

Regards

Ian Barwick

On Fri, Dec 2, 2022 at 11:29 PM Ian Lawrence Barwick <barwick@gmail.com> wrote:

Though on reflection maybe it's overkill and the existing tests
suffice. Anyway leaving the patch here in the interests of pushing
this forward in some direction.

Do you think that there is a scenario where 008_untar.pl and
009_extract.pl pass but this test fails, alerting us to a problem that
would otherwise have gone undetected? If so, what is that scenario?

The only thing that I can think of would be if $decompress_program
--test were failing, but actually trying to decompress succeeded. I
would be inclined to dismiss that particular scenario as not important
enough to be worth the additional CPU cycles.

--
Robert Haas
EDB: http://www.enterprisedb.com

2022年12月5日(月) 23:59 Robert Haas <robertmhaas@gmail.com>:

On Fri, Dec 2, 2022 at 11:29 PM Ian Lawrence Barwick <barwick@gmail.com> wrote:

Though on reflection maybe it's overkill and the existing tests
suffice. Anyway leaving the patch here in the interests of pushing
this forward in some direction.

Do you think that there is a scenario where 008_untar.pl and
009_extract.pl pass but this test fails, alerting us to a problem that
would otherwise have gone undetected? If so, what is that scenario?

The only thing that I can think of would be if $decompress_program
--test were failing, but actually trying to decompress succeeded. I
would be inclined to dismiss that particular scenario as not important
enough to be worth the additional CPU cycles.

Yeah, it doesn't really add anything, so let's close this one off.

Thanks for the feedback

Ian Barwick

The only thing that I can think of would be if $decompress_program
--test were failing, but actually trying to decompress succeeded. I
would be inclined to dismiss that particular scenario as not important
enough to be worth the additional CPU cycles.

When I wrote this test, it was just to increase coverage for pg_basebackup.
As I checked again, it already does that in the pg_verifybackup
008_untar.pl, 009_extrack.pl test you mentioned.
Therefore, I agree with your opinion.

---
Regards,
DongWook Lee.