Re: Stronger safeguard for archive recovery not to miss data

On 3/25/21 9:23 PM, Fujii Masao wrote:
>
> On 2021/03/25 23:21, David Steele wrote:
>> On 1/25/21 3:55 AM, Laurenz Albe wrote:
>>> On Mon, 2021-01-25 at 08:19 +0000, osumi(dot)takamichi(at)fujitsu(dot)com wrote:
>>>>> I think you should pst another patch where the second, now
>>>>> superfluous, error
>>>>> message is removed.
>>>>
>>>> Updated. This patch showed no failure during regression tests
>>>> and has been aligned by pgindent.
>>>
>>> Looks good to me.
>>> I'll set it to "ready for committer" again.
>>
>> Fujii, does the new patch in [1] address your concerns?
>
> No. I'm still not sure if this patch is good idea... I understand
> why this safeguard is necessary. OTOH I'm afraid it increases
> a bit the risk that users get unstartable database, i.e., lose whole
> database.
> But maybe I'm concerned about rare case and my opinion is minority one.
> So I'd like to hear more opinions about this patch.

After reviewing the patch I am +1. I think allowing corruption in
recovery by default is not a good idea. There is currently a warning but
I don't think that is nearly strong enough and is easily missed.

Also, "data may be missing" makes this sound like an acceptable
situation. What is really happening is corruption is being introduced
that may make the cluster unusable or at the very least lead to errors
during normal operation.

If we want to allow recovery to play past this point I think it would
make more sense to have a GUC (like ignore_invalid_pages [1]) that
allows recovery to proceed and emits a warning instead of fatal.

Looking the patch, I see a few things:

1) Typo in the tests:

This protection shold apply to recovery mode

should be:

This protection should apply to recovery mode

2) I don't think it should be the job of this patch to restructure the
if conditions, even if it is more efficient. It just obscures the
purpose of the patch. So, I would revert all the changes in xlog.c
except changing the warning to an error:

- ereport(WARNING,
- (errmsg("WAL was generated with wal_level=minimal, data may be
missing"),
- errhint("This happens if you temporarily set wal_level=minimal
without taking a new base backup.")));
+ ereport(FATAL,
+ (errmsg("WAL was generated with wal_level=minimal, cannot continue
recovering"),
+ errdetail("This happens if you temporarily set wal_level=minimal
on the server."),
+ errhint("Run recovery again from a new base backup taken after
setting wal_level higher than minimal")));

--
-David
david(at)pgmasters(dot)net

[1]
https://www.postgresql.org/message-id/E1iu6D8-0000tK-Cm%40gemulon.postgresql.org