Stronger safeguard for archive recovery not to miss data

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
55 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

David Steele
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, [hidden email] 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
[hidden email]

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


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Fujii Masao-4


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, [hidden email] 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.")));

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <[hidden email]> wrote in

>
>
> 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, [hidden email] 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.

Sorry for being late, but FWIW, I'm confused.

This patch checks if archive recovery is requested after shutting down
while in the minimal mode.  Since we officially reject to take a
backup while wal_level=minimal, the error is not supposed to be raised
while recoverying from a past backup.  If the cluster was running in
minimal mode, the archive recovery does nothing substantial (would end
with running a crash recvoery at worst).  I'm not sure how the
concrete curruption mode caused by the operation looks like.  I agree
that we should reject archive recovery on a wal_level=minimal cluster,
but the reason is not that the operation is breaking the past backup,
but that it's just nonsentical.

On the other hand, I don't think this patch effectively protect
archive from getting a stopping gap. Just starting the server without
archive recovery succeeds and the archive is successfully broken for
the backups in the past.  In other words, if we are intending to let
users know that their backups have gotten a stopping gap, this patch
doesn't seem to work in that sense.  I would object to reject starting
with wal_level=replica and without archive recovery after shutting
down in minimal mode. That's obviously an overkill.

So, what I think we should do for the objective is to warn if
archiving is enabled but backup is not yet taken.  Yeah, (as I
remember that such discussion has been happened) I don't think we have
a reliable way to know that.

I apologize in advance if I'm missing something.

> > 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 don't think even this is needed.

> > 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.")));

It seems to be alredy removed in the latest patch?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
At Wed, 31 Mar 2021 14:23:26 +0900 (JST), Kyotaro Horiguchi <[hidden email]> wrote in
> I apologize in advance if I'm missing something.

Oops! I missed the case of replica side.  It's obviously useless that
replica continue to run allowing a stopping gap made by
wal_level=minimal.

So, I don't object to this patch.

Sorry for the noise.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
In reply to this post by Fujii Masao-4
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <[hidden email]> wrote in

>
>
> 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, [hidden email] 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.

So +1 from me in its direction.

> > 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?

I agree that the hint doesn't make sense.

HINT:  Restart with archive recovery turned off.  The past backups are no longer usable.  You need to take a new one after restart.

If it's the replica case, it would be..

HINT:  Start from a fresh standby created from the curent primary server.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi <[hidden email]> wrote in

> At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <[hidden email]> 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.

For the primary case,

> HINT:  Restart with archive recovery turned off.  The past backups are no longer usable.  You need to take a new one after restart.
>
> If it's the replica case, it would be..
>
> HINT:  Start from a fresh standby created from the curent primary server.

Start from a fresh backup...

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

osumi.takamichi@fujitsu.com
Hi,


On Wednesday, March 31, 2021 3:06 PM Kyotaro Horiguchi <[hidden email]> wrote:

> At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi
> <[hidden email]> wrote in
> > At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao
> > <[hidden email]> 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.
>
> For the primary case,
>
> > HINT:  Restart with archive recovery turned off.  The past backups are no
> longer usable.  You need to take a new one after restart.
> >
> > If it's the replica case, it would be..
> >
> > HINT:  Start from a fresh standby created from the curent primary server.
>
> Start from a fresh backup...
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.

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/backend/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.


[1] - https://www.mail-archive.com/pgsql-hackers@.../msg323147.html


Best Regards,
        Takamichi Osumi


stronger_safeguard_for_archive_recovery_v05.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Fujii Masao-4


On 2021/04/01 12:45, [hidden email] wrote:

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

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?

+# Check if standby.signal exists
+my $pgdata = $new_node->data_dir;
+ok (-f "${pgdata}/standby.signal", 'standby.signal was created');

+# Check if recovery.signal exists
+my $path = $another_node->data_dir;
+ok (-f "${path}/recovery.signal", 'recovery.signal was created');

Why are these tests necessary?
These seem to test that init_from_backup() works expectedly based on
the parameter "standby". But if we are sure that init_from_backup() works fine,
these tests don't seem to be necessary.

+use Config;

This is not necessary?

+# Make the wal_level back to replica
+$node->append_conf('postgresql.conf', $replica_config);
+$node->restart;
+check_wal_level('replica', 'wal_level went back to replica again');

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.

 
> 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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Laurenz Albe
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.

Yours,
Laurenz Albe



Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

osumi.takamichi@fujitsu.com
In reply to this post by Fujii Masao-4
On Thursday, April 1, 2021 5:25 PM Fujii Masao <[hidden email]> wrote:

> On 2021/04/01 12:45, [hidden email] wrote:
> > 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.
>
> 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?
Adopted.


> +# Check if standby.signal exists
> +my $pgdata = $new_node->data_dir;
> +ok (-f "${pgdata}/standby.signal", 'standby.signal was created');
>
> +# Check if recovery.signal exists
> +my $path = $another_node->data_dir;
> +ok (-f "${path}/recovery.signal", 'recovery.signal was created');
>
> Why are these tests necessary?
> These seem to test that init_from_backup() works expectedly based on the
> parameter "standby". But if we are sure that init_from_backup() works fine,
> these tests don't seem to be necessary.
Absolutely, you are right. Fixed.


> +use Config;
>
> This is not necessary?
Removed.


> +# Make the wal_level back to replica
> +$node->append_conf('postgresql.conf', $replica_config); $node->restart;
> +check_wal_level('replica', 'wal_level went back to replica again');
>
> 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.


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


Best Regards,
        Takamichi Osumi


stronger_safeguard_for_archive_recovery_v06.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

osumi.takamichi@fujitsu.com
In reply to this post by Laurenz Albe
On  Friday, April 2, 2021 11:49 PM  Laurenz Albe <[hidden email]> wrote:

> 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.
Thank you. Yet, I prefer the longer version.
For example, the later backup can be another backup that fails during archive recovery
if the user have several backups during wal_level=replica
and it is taken before setting wal_level=minimal, right ?

Like this, giving much information is helpful for better decision taken by user, I thought.

Best Regards,
        Takamichi Osumi

Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Fujii Masao-4
In reply to this post by osumi.takamichi@fujitsu.com


On 2021/04/04 11:58, [hidden email] 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.

+# This protection should apply to recovery mode
+my $another_node = get_new_node('another_node');

The same test is performed twice with different settings. So isn't it better
to merge the code for both tests into one common function, to simplify
the code? I did that.

I also applied some cosmetic changes.


>>> 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?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

stronger_safeguard_for_archive_recovery_v07.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

tsunakawa.takay@fujitsu.com
In reply to this post by osumi.takamichi@fujitsu.com
From: [hidden email] <[hidden email]>

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



Regards
Takayuki Tsunakawa


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
In reply to this post by Fujii Masao-4
At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao <[hidden email]> wrote in

>
>
> On 2021/04/04 11:58, [hidden email] 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.  The caller knows the LSN of last PARAMETER_CHANGE
record so we can show that as the recovery-end LSN.  On the other
hand, I'm not sure it's worth considering tough, we can reach here
when starting archive recovery on a server with wal_level=minimal.  We
can pass 0 as LSN to notify that case.

If we do that, we can emit more clear message like the following.

WAL-while-minimal case
 FATAL:  WAL generated with wal_level=minimal at 0/40000A0, data may be missing
 HINT:  Use a backup later than, or recover up to before that LSN.

Mis-setting case
 FATAL:  archive recovery is not available on server with wal_level=minimal
 HINT:  Start this server with setting wal_level=replica or higher


The value "replica" is not double-quoted there (since before this
patch), but double-quoted in another error message about hot standby
just below.  Maybe we should let them in the same style.

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

+1.

> +# This protection should apply to recovery mode
> +my $another_node = get_new_node('another_node');
>
> The same test is performed twice with different settings. So isn't it
> better
> to merge the code for both tests into one common function, to simplify
> the code? I did that.

Sounds reasonable for that size.


regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Kyotaro Horiguchi-4
In reply to this post by tsunakawa.takay@fujitsu.com
At Mon, 5 Apr 2021 07:04:09 +0000, "[hidden email]" <[hidden email]> wrote in

> From: [hidden email] <[hidden email]>
> > 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.

I didn't see that, but found the following article.

https://stackoverflow.com/questions/2590794/gcov-warning-merge-mismatch-for-summaries

> This happens when one of the objects you're linking into an executable
> changes significantly. For example it gains or loses some lines of
> profilable code.

It seems like your working directory needs some cleanup.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

tsunakawa.takay@fujitsu.com
From: Kyotaro Horiguchi <[hidden email]>
> I didn't see that, but found the following article.
>
> https://stackoverflow.com/questions/2590794/gcov-warning-merge-mismat
> ch-for-summaries
...
> It seems like your working directory needs some cleanup.

That could very well be.  It'd be safer to run "make coverage-clean" between builds.

I thought otherwise that the multiple TAP tests that were simultaneously run conflicted on the .gcda file.  If this is the case, we may not be able to eliminate the failure possibility.  (make -J 1 circumvents this?)

https://www.postgresql.org/docs/devel/install-procedure.html

"If using GCC, all programs and libraries are compiled with code coverage testing instrumentation."

33.4. TAP Tests
https://www.postgresql.org/docs/devel/regress-tap.html

"Generically speaking, the TAP tests will test the executables in a previously-installed installation tree if you say make installcheck, or will build a new local installation tree from current sources if you say make check. In either case they will initialize a local instance (data directory) and transiently run a server in it. Some of these tests run more than one server."

"It's important to realize that the TAP tests will start test server(s) even when you say make installcheck; this is unlike the traditional non-TAP testing infrastructure, which expects to use an already-running test server in that case. Some PostgreSQL subdirectories contain both traditional-style and TAP-style tests, meaning that make installcheck will produce a mix of results from temporary servers and the already-running test server."



Regards
Takayuki Tsunakawa




Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

Fujii Masao-4
In reply to this post by Kyotaro Horiguchi-4


On 2021/04/05 16:13, Kyotaro Horiguchi wrote:

> At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao <[hidden email]> wrote in
>>
>>
>> On 2021/04/04 11:58, [hidden email] 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?

[1]
BTW, one very tricky way to recover from this situation seems to
copy all required WAL files from the archive to pg_wal and forcibly
run a crash recovery from the backup. Since crash recovery doesn't
check wal_level, we can avoid the issue by doing that. But this is
very tricky.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

osumi.takamichi@fujitsu.com
In reply to this post by Fujii Masao-4
On Mon Apr 5, 2021 12:35 PM Fujii Masao <[hidden email]> wrote:

> On 2021/04/04 11:58, [hidden email] 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.
Yeah, at the phase to commit, we don't need those.
Let's make only essential parts left.


> +# This protection should apply to recovery mode my $another_node =
> +get_new_node('another_node');
>
> The same test is performed twice with different settings. So isn't it better to
> merge the code for both tests into one common function, to simplify the
> code? I did that.
>
> I also applied some cosmetic changes.
Thank you so much. Your comments are by far more precise.
Further, the refactoring of the two tests by the common function looks really good.

Some minor comments.

(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" ?

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

(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 ?


> >>> 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?
v07 reproduces the phenomena, even with make coverage-clean between tests.
The possibility is not high though.

We cannot do the regression test separately from xlog.c
because it uses the new error message of xlog.c.
Applying only the TAP test should fail because
we get an warning not error.

Therefore, I took the changes of xlog.c only and
I'm doing the RT in a loop now. If we can get the stderr again,
then we can guess xlog.c is the cause, right ?

I think I can report the result tomorrow.
Just in case, I'm running the RT for OSS HEAD in parallel...
although I cannot reproduce it with it at all.


Best Regards,
        Takamichi Osumi

Reply | Threaded
Open this post in threaded view
|

RE: Stronger safeguard for archive recovery not to miss data

osumi.takamichi@fujitsu.com
In reply to this post by Fujii Masao-4
On  Monday, April 5, 2021 9:16 PM Fujii Masao <[hidden email]> wrote:

> On 2021/04/05 16:13, Kyotaro Horiguchi wrote:
> > At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao
> > <[hidden email]> wrote in
> >>
> >>
> >> On 2021/04/04 11:58, [hidden email] 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?
+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.


Best Regards,
        Takamichi Osumi

Reply | Threaded
Open this post in threaded view
|

Re: Stronger safeguard for archive recovery not to miss data

David Steele
In reply to this post by Fujii Masao-4
On 4/4/21 11:34 PM, Fujii Masao wrote:

>
> On 2021/04/04 11:58, [hidden email] 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.

I'm good with this patch as is. I would rather not bike shed the hint
too much as time is short to get this patch in.

Regards,
--
-David
[hidden email]


123