Stronger safeguard for archive recovery not to miss data

>
> 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, 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.

> 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, 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.

>
>
> On 2021/03/26 22:14, David Steele wrote:
> > 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, 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.
>
> Ok, now you, Osumi-san and Laurenz agree to this change
> while I'm only the person not to like this. So unless we can hear
> any other objections to this change, probably we should commit the
> patch.

> > 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.
>
> +1

> > 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")));
> I guess that users usually encounter this error because they have not
> taken base backups yet after setting wal_level to higher than minimal
> and have to use the old base backup for archive recovery. So I'm not
> sure
> how much only this HINT is helpful for them. Isn't it better to append
> something like "If there is no such backup, recover to the point in
> time
> before wal_level is set to minimal even though which cause data loss,
> to start the server." into HINT?
> The following error will never be thrown because of the patch.
> So IMO the following code should be removed.
>
> if (ControlFile->wal_level < WAL_LEVEL_REPLICA)
> ereport(ERROR,
> (errmsg("hot standby is not possible because wal_level was
> not set to \"replica\" or higher on the primary server"),
> errhint("Either set wal_level to \"replica\" on the
> primary, or turn off hot_standby here.")));

>
>
> On 2021/03/26 22:14, David Steele wrote:
> > 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, 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.
>
> Ok, now you, Osumi-san and Laurenz agree to this change
> while I'm only the person not to like this. So unless we can hear
> any other objections to this change, probably we should commit the
> patch.

> > 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.
>
> +1
>
> > 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")));
> I guess that users usually encounter this error because they have not
> taken base backups yet after setting wal_level to higher than minimal
> and have to use the old base backup for archive recovery. So I'm not
> sure
> how much only this HINT is helpful for them. Isn't it better to append
> something like "If there is no such backup, recover to the point in
> time
> before wal_level is set to minimal even though which cause data loss,
> to start the server." into HINT?

> At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao < > wrote in
> > > 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")));
> > I guess that users usually encounter this error because they have not
> > taken base backups yet after setting wal_level to higher than minimal
> > and have to use the old base backup for archive recovery. So I'm not
> > sure
> > how much only this HINT is helpful for them. Isn't it better to append
> > something like "If there is no such backup, recover to the point in
> > time
> > before wal_level is set to minimal even though which cause data loss,
> > to start the server." into HINT?
>
> I agree that the hint doesn't make sense.

Thank you for sharing your ideas about the hint. Absolutely need to change the message.

> Thank you for sharing your ideas about the hint. Absolutely need to change the message.
> In my opinion, combining the basic idea of yours and Fujii-san's would be the best.
>
> Updated the patch and made v05. The changes I made are
>
> * rewording of errhint although this has become long !
> * fix of the typo in the TAP test
> * modification of my past changes not to change conditions in CheckRequiredParameterValues
> * rename of the test file to 024_archive_recovery.pl because two files are made
> since the last update of this patch
> * pgindent is conducted to check my alignment again.

Adopted.

Absolutely, you are right. Fixed.

> On Thu, 2021-04-01 at 17:25 +0900, Fujii Masao wrote:
> > Thanks for updating the patch!
> >
> > +                                errhint("Use a backup taken after
> setting wal_level to higher than minimal "
> > +                                                "or recover to the
> > + point in time before wal_level becomes minimal even though it causes
> > + data loss")));
> >
> > ISTM that "or recover to the point in time before wal_level was changed
> >   to minimal even though it may cause data loss" sounds better. Thought?
>
> I would reduce it to
>
> "Either use a later backup, or recover to a point in time before \"wal_level\"
> was set to \"minimal\"."
>
> I'd say that we can leave it to the intelligence of the reader to deduce that
> recovering to an earlier time means more data loss.

> By the way, when I build postgres with this patch and enable-coverage option,
> the results of RT becomes unstable. Does someone know the reason ?
> When it fails, I get stderr like below
>
> t/001_start_stop.pl .. 10/24
> #   Failed test 'pg_ctl start: no stderr'
> #   at t/001_start_stop.pl line 48.
> #          got:
> 'profiling:/home/k5user/new_disk/recheck/PostgreSQL-Source-Dev/src/bac
> kend/executor/execMain.gcda:Merge mismatch for function 15
> # '
> #     expected: ''
> t/001_start_stop.pl .. 24/24 # Looks like you failed 1 test of 24.
> t/001_start_stop.pl .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/24
> subtests
>
> Similar phenomena was observed in [1] and its solution seems to upgrade my
> gcc higher than 7. And, I did so but still get this unstable error with
> enable-coverage. This didn't happen when I remove enable-option and the
> make check-world passes.

>
>
> On 2021/04/04 11:58, wrote:
> >> IMO it's better to comment why this server restart is necessary.
> >> As far as I understand correctly, this is necessary to ensure the WAL
> >> file
> >> containing the record about the change of wal_level (to minimal) is
> >> archived,
> >> so that the subsequent archive recovery will be able to replay it.
> > OK, added some comments. Further, I felt the way I wrote this part was
> > not good at all and self-evident
> > and developers who read this test would feel uneasy about that point.
> > So, a little bit fixed that test so that we can get clearer conviction
> > for wal archive.
>
> LGTM. Thanks for updating the patch!
>
> Attached is the updated version of the patch. I applied the following
> changes.

> Could you review this version? Barring any objection, I'm thinking to
> commit this.
>
> +sub check_wal_level
> +{
> +my ($target_wal_level, $explanation) = @_;
> +
> +is( $node->safe_psql(
> +'postgres', q{show wal_level}),
> +        $target_wal_level,
> +        $explanation);
>
> Do we really need this test? This test doesn't seem to be directly
> related
> to the test purpose. It seems to be testing the behavior of the
> PostgresNode
> functions. So I removed this from the patch.

> From: < >
> > By the way, when I build postgres with this patch and enable-coverage option,
> > the results of RT becomes unstable. Does someone know the reason ?
> > When it fails, I get stderr like below
> >
> > t/001_start_stop.pl .. 10/24
> > #   Failed test 'pg_ctl start: no stderr'
> > #   at t/001_start_stop.pl line 48.
> > #          got:
> > 'profiling:/home/k5user/new_disk/recheck/PostgreSQL-Source-Dev/src/bac
> > kend/executor/execMain.gcda:Merge mismatch for function 15
> > # '
> > #     expected: ''
> > t/001_start_stop.pl .. 24/24 # Looks like you failed 1 test of 24.
> > t/001_start_stop.pl .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/24
> > subtests
> >
> > Similar phenomena was observed in [1] and its solution seems to upgrade my
> > gcc higher than 7. And, I did so but still get this unstable error with
> > enable-coverage. This didn't happen when I remove enable-option and the
> > make check-world passes.
>
> Can you share the steps you took?  e.g.,
>
> $ configure --enable-coverage ...
> $ make world
> $ make check-world
> $ patch -p1 < your_patch
> $ make world
> $ make check-world
>
> A bit of Googling shows that the same error message has shown up in the tests of other software than Postgres.  It doesn't seem like this failure is due to your patch.

> At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao < > wrote in
>>
>>
>> On 2021/04/04 11:58, wrote:
>>>> IMO it's better to comment why this server restart is necessary.
>>>> As far as I understand correctly, this is necessary to ensure the WAL
>>>> file
>>>> containing the record about the change of wal_level (to minimal) is
>>>> archived,
>>>> so that the subsequent archive recovery will be able to replay it.
>>> OK, added some comments. Further, I felt the way I wrote this part was
>>> not good at all and self-evident
>>> and developers who read this test would feel uneasy about that point.
>>> So, a little bit fixed that test so that we can get clearer conviction
>>> for wal archive.
>>
>> LGTM. Thanks for updating the patch!
>>
>> Attached is the updated version of the patch. I applied the following
>> changes.
>
> + errhint("Use a backup taken after setting wal_level to higher than minimal "
> + "or recover to the point in time before wal_level was changed to minimal even though it may cause data loss.")));
>
> Looking the HINT message, I thought that it's hard to find where up to
> I should recover.

> On 2021/04/04 11:58, wrote:
> >> IMO it's better to comment why this server restart is necessary.
> >> As far as I understand correctly, this is necessary to ensure the WAL
> >> file containing the record about the change of wal_level (to minimal)
> >> is archived, so that the subsequent archive recovery will be able to replay
> it.
> > OK, added some comments. Further, I felt the way I wrote this part was
> > not good at all and self-evident and developers who read this test would
> feel uneasy about that point.
> > So, a little bit fixed that test so that we can get clearer conviction for wal
> archive.
>
> LGTM. Thanks for updating the patch!
>
> Attached is the updated version of the patch. I applied the following changes.
> Could you review this version? Barring any objection, I'm thinking to commit
> this.
>
> +sub check_wal_level
> +{
> +my ($target_wal_level, $explanation) = @_;
> +
> +is( $node->safe_psql(
> +'postgres', q{show wal_level}),
> +        $target_wal_level,
> +        $explanation);
>
> Do we really need this test? This test doesn't seem to be directly related to
> the test purpose. It seems to be testing the behavior of the PostgresNode
> functions. So I removed this from the patch.

> >>> By the way, when I build postgres with this patch and
> >>> enable-coverage option, the results of RT becomes unstable. Does
> >>> someone know the
> >> reason ?
> >>> When it fails, I get stderr like below
> >>
> >> I have no idea about this. Does this happen even without the patch?
> > Unfortunately, no. I get this only with --enable-coverage and with my
> > patch, althought regression tests have passed with this patch.
> > OSS HEAD doesn't produce the stderr even with --enable-coverage.
>
> Could you check whether the latest patch still causes this issue or not?
> If it still causes, could you check which part (the change of xlog.c or the
> addition of regression test) caused the issue?

> On 2021/04/05 16:13, Kyotaro Horiguchi wrote:
> > At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao
> > < > wrote in
> >>
> >>
> >> On 2021/04/04 11:58, wrote:
> >>>> IMO it's better to comment why this server restart is necessary.
> >>>> As far as I understand correctly, this is necessary to ensure the
> >>>> WAL file containing the record about the change of wal_level (to
> >>>> minimal) is archived, so that the subsequent archive recovery will
> >>>> be able to replay it.
> >>> OK, added some comments. Further, I felt the way I wrote this part
> >>> was not good at all and self-evident and developers who read this
> >>> test would feel uneasy about that point.
> >>> So, a little bit fixed that test so that we can get clearer
> >>> conviction for wal archive.
> >>
> >> LGTM. Thanks for updating the patch!
> >>
> >> Attached is the updated version of the patch. I applied the following
> >> changes.
> >
> > + errhint("Use a backup taken after setting
> wal_level to higher than minimal "
> > + "or recover to the point in
> time before wal_level was changed
> > +to minimal even though it may cause data loss.")));
> >
> > Looking the HINT message, I thought that it's hard to find where up to
> > I should recover.
>
> Yes. And, what's the worse, when archive recovery finds WAL generated with
> wal_level=minimal and fails, "minimal" is saved in pg_control's wal_level.
> This means that subsequent archive recovery always fails at the beginning of
> recovery (before entering WAL replay main loop), in that case.
> So even if recovery_targrt_lsn is specified, archive recovery fails before
> checking that. Any recovery target settings have no effect on that case.
>
> Maybe we can avoid this, for example, by changing xlog_redo() so that it calls
> CheckRequiredParameterValues() before UpdateControlFile().
> But I'm not sure if this change is safe. Probably we need more time to
> consider this, but right now there is no so much time left at this stage.
>
> At least the HINT message "or recover to the point in time before wal_level
> was changed to minimal even though it may cause data loss." should be
> removed because it's not helpful at all...
>
> Ok, so if archive recovery finds WAL generated with wal_level=minimal and
> fails, and also there is no backup taken after wal_level is set to higher than
> minimal, basically [1] we lose whole database. I think that those who set
> wal_level to minimal understand that this setting can cause data loss, for
> example, any data loaded with wal_level=minimal may be lost later. But I'm
> afraid that they might not understand the risk of whole database loss.
>
> Even if they take new backup just after they set wal_level to higher than
> minimal, there is still the risk of whole database loss until the backup is
> completed.
>
> This makes me think that we should document this risk.... Thought?

>
> On 2021/04/04 11:58, wrote:
>>> IMO it's better to comment why this server restart is necessary.
>>> As far as I understand correctly, this is necessary to ensure the WAL
>>> file
>>> containing the record about the change of wal_level (to minimal) is
>>> archived,
>>> so that the subsequent archive recovery will be able to replay it.
>> OK, added some comments. Further, I felt the way I wrote this part was
>> not good at all and self-evident
>> and developers who read this test would feel uneasy about that point.
>> So, a little bit fixed that test so that we can get clearer conviction
>> for wal archive.
>
> LGTM. Thanks for updating the patch!
>
> Attached is the updated version of the patch. I applied the following
> changes.
> Could you review this version? Barring any objection, I'm thinking to
> commit this.