On 2021/04/05 23:49, osumi.takamichi@fujitsu.com wrote:
> Some minor comments.
Thanks for the review!
> (1)
>
> + # Confirm that the archive recovery fails with an error
> + my $logfile = slurp_file($recovery_node->logfile());
> + ok( $logfile =~
> + qr/FATAL: WAL was generated with wal_level=minimal, cannot continue recovering/,
> + "$node_text ends with an error because it finds WAL generated with wal_level=minimal");
>
> How about a comment
> "Confirm that the archive recovery fails with an expected error" ?
Sounds good to me. Fixed.
> (2)
>
> +# Test for archive recovery
> +test_recovery_wal_level_minimal('archive_recovery', 'archive recovery', 0);
> We have two spaces in one comment. Should be fixed.
Yes, fixed.
> (3)
>
> I thought the function name 'test_recovery_wal_level_minimal'
> is a little bit weird and can be changed.
> How about something like execute_recovery_scenario,
> test_recovery_safeguard or test_archive_recovery_safeguard ?
I prefer the original name, so I didn't change this.
And we can rename it later if necessary.
On 2021/04/05 23:54, osumi.takamichi@fujitsu.com wrote:
>> This makes me think that we should document this risk.... Thought?
> +1. We should notify the risk when user changes
> the wal_level higher than minimal to minimal
> to invoke a carefulness of user for such kind of operation.
I removed the HINT message "or recover to the point in ..." and
added the following note into the docs.
Note that changing <varname>wal_level</varname> to
<literal>minimal</literal> makes any base backups taken before
unavailable for archive recovery and standby server, which may
lead to database loss.
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION