Обсуждение: Question about 001_stream_rep.pl recovery test

Hi Hackers,

Inside the test script `src/test/recovery/t/001_stream_rep.pl`, a 
comment at line 30 says `my_backup` is "not mandatory",

  30 # Take backup of standby 1 (not mandatory, but useful to check if
  31 # pg_basebackup works on a standby).
  32 $node_standby_1->backup($backup_name);

however if remove the backup folder "my_backup" after line 32, something 
like below,

  33 system_or_bail('rm', '-rf', 
'/home/user/postgres/src/test/recovery/tmp_check/t_001_stream_rep_standby_1_data/backup/my_backup');

then the test failed with message like,

recovery$ make check PROVE_TESTS='t/001_stream_rep.pl'
t/001_stream_rep.pl .. # Looks like your test exited with 2 before it 
could output anything.
t/001_stream_rep.pl .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 53/53 subtests
... ...
Result: FAIL
Makefile:23: recipe for target 'check' failed
make: *** [check] Error 1

And then the test script takes another backup `my_backup_2`, but it 
seems this backup is not used anywhere.

  35 # Take a second backup of the standby while the primary is offline.
  36 $node_primary->stop;
  37 $node_standby_1->backup('my_backup_2');
  38 $node_primary->start;

because, if I deleted the backup "my_backup_2" right after line 38 with 
something like below,

39 system_or_bail('rm', '-rf', 
'/home/user/postgres/src/test/recovery/tmp_check/t_001_stream_rep_standby_1_data/backup/my_backup_2');
there is no impact on the test result, and it says all test cases passed.

recovery$ make check PROVE_TESTS='t/001_stream_rep.pl'
...
t/001_stream_rep.pl .. ok
All tests successful.
Files=1, Tests=53,  6 wallclock secs ( 0.01 usr  0.01 sys + 1.00 cusr  
0.82 csys =  1.84 CPU)
Result: PASS

Did I misunderstand something here?

Thank you,

-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca

On Fri, Dec 10, 2021 at 01:44:40PM -0800, David Zhang wrote:
> Inside the test script `src/test/recovery/t/001_stream_rep.pl`, a comment at
> line 30 says `my_backup` is "not mandatory",
>
>  30 # Take backup of standby 1 (not mandatory, but useful to check if
>  31 # pg_basebackup works on a standby).
>  32 $node_standby_1->backup($backup_name);
>
> however if remove the backup folder "my_backup" after line 32, something
> like below,

Well, the comment is not completely incorrect IMO.  In this context, I
read taht that we don't need a backup from a standby and we could just
take a backup from the primary instead.  If I were to fix something, I
would suggest to reword the comment as follows:
# Take backup of standby 1 (could be taken from the primary, but this
# is useful to check if pg_basebackup works on a standby).

> And then the test script takes another backup `my_backup_2`, but it seems
> this backup is not used anywhere.

This had better stay around, per commit f267c1c.  And as far as I can
see, we don't have an equivalent test in a different place, where
pg_basebackup runs on a standby while its *primary* is stopped.
--
Michael

Thanks a lot Michael for the explanation.

On 2021-12-13 4:05 a.m., Michael Paquier wrote:
> On Fri, Dec 10, 2021 at 01:44:40PM -0800, David Zhang wrote:
>> Inside the test script `src/test/recovery/t/001_stream_rep.pl`, a comment at
>> line 30 says `my_backup` is "not mandatory",
>>
>>   30 # Take backup of standby 1 (not mandatory, but useful to check if
>>   31 # pg_basebackup works on a standby).
>>   32 $node_standby_1->backup($backup_name);
>>
>> however if remove the backup folder "my_backup" after line 32, something
>> like below,
> Well, the comment is not completely incorrect IMO.  In this context, I
> read taht that we don't need a backup from a standby and we could just
> take a backup from the primary instead.  If I were to fix something, I
> would suggest to reword the comment as follows:
> # Take backup of standby 1 (could be taken from the primary, but this
> # is useful to check if pg_basebackup works on a standby).
>
>> And then the test script takes another backup `my_backup_2`, but it seems
>> this backup is not used anywhere.
> This had better stay around, per commit f267c1c.  And as far as I can
> see, we don't have an equivalent test in a different place, where
> pg_basebackup runs on a standby while its *primary* is stopped.
> --
> Michael
-- 
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca