Thread: pg_basebackup: add test about zstd compress option
Hi hackers, 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. --- Regards, DongWook Lee.
Attachment
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
Attachment
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?
Attachment
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] 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
Attachment
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.