Re: Stronger safeguard for archive recovery not to miss data

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