Re: Stronger safeguard for archive recovery not to miss data

On 2021/04/05 23:49, osumi(dot)takamichi(at)fujitsu(dot)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(dot)takamichi(at)fujitsu(dot)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