Обсуждение: Stronger safeguard for archive recovery not to miss data
Hello
The attached patch is intended to prevent a scenario that
archive recovery hits WALs which come from wal_level=minimal
and the server continues to work, which was discussed in the thread of [1].
The motivation is to protect that user ends up with both getting replica
that could miss data and getting the server to miss data in targeted recovery mode.
About how to modify this, we reached the consensus in the thread.
It is by changing the ereport's level from WARNING to FATAL in CheckRequiredParameterValues().
In order to test this fix, what I did is
1 - get a base backup during wal_level is replica
2 - stop the server and change the wal_level from replica to minimal
3 - restart the server(to generate XLOG_PARAMETER_CHANGE)
4 - stop the server and make the wal_level back to replica
5 - start the server again
6 - execute archive recoveries in both cases
(1) by editing the postgresql.conf and
touching recovery.signal in the base backup from 1th step
(2) by making a replica with standby.signal
* During wal_level is replica, I enabled archive_mode in this test.
First of all, I confirmed the server started up without this patch.
After applying this safeguard patch, I checked that
the server cannot start up any more in the scenario case.
I checked the log and got the result below with this patch.
2020-11-26 06:49:46.003 UTC [19715] FATAL: WAL was generated with wal_level=minimal, data may be missing
2020-11-26 06:49:46.003 UTC [19715] HINT: This happens if you temporarily set wal_level=minimal without taking a new
basebackup.
Lastly, this should be backpatched.
Any comments ?
[1]
https://www.postgresql.org/message-id/TYAPR01MB29901EBE5A3ACCE55BA99186FE320%40TYAPR01MB2990.jpnprd01.prod.outlook.com
Best,
Takamichi Osumi
Вложения
At Thu, 26 Nov 2020 07:18:39 +0000, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> wrote in > Hello > > > The attached patch is intended to prevent a scenario that > archive recovery hits WALs which come from wal_level=minimal > and the server continues to work, which was discussed in the thread of [1]. > The motivation is to protect that user ends up with both getting replica > that could miss data and getting the server to miss data in targeted recovery mode. > > About how to modify this, we reached the consensus in the thread. > It is by changing the ereport's level from WARNING to FATAL in CheckRequiredParameterValues(). > > In order to test this fix, what I did is > 1 - get a base backup during wal_level is replica > 2 - stop the server and change the wal_level from replica to minimal > 3 - restart the server(to generate XLOG_PARAMETER_CHANGE) > 4 - stop the server and make the wal_level back to replica > 5 - start the server again > 6 - execute archive recoveries in both cases > (1) by editing the postgresql.conf and > touching recovery.signal in the base backup from 1th step > (2) by making a replica with standby.signal > * During wal_level is replica, I enabled archive_mode in this test. > > First of all, I confirmed the server started up without this patch. > After applying this safeguard patch, I checked that > the server cannot start up any more in the scenario case. > I checked the log and got the result below with this patch. > > 2020-11-26 06:49:46.003 UTC [19715] FATAL: WAL was generated with wal_level=minimal, data may be missing > 2020-11-26 06:49:46.003 UTC [19715] HINT: This happens if you temporarily set wal_level=minimal without taking a new basebackup. > > Lastly, this should be backpatched. > Any comments ? Perhaps we need the TAP test that conducts the above steps. > [1] > https://www.postgresql.org/message-id/TYAPR01MB29901EBE5A3ACCE55BA99186FE320%40TYAPR01MB2990.jpnprd01.prod.outlook.com regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hello > First of all, I confirmed the server started up without this patch. It is possible only with manually configured hot_standby = off, right? We have ERROR in hot standby mode just below in CheckRequiredParameterValues. regards, Sergei
Hello, Horiguchi-San
On Thursday, Nov 26, 2020 4:29 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> At Thu, 26 Nov 2020 07:18:39 +0000, "osumi.takamichi@fujitsu.com"
> <osumi.takamichi@fujitsu.com> wrote in
> > In order to test this fix, what I did is
> > 1 - get a base backup during wal_level is replica
> > 2 - stop the server and change the wal_level from replica to minimal
> > 3 - restart the server(to generate XLOG_PARAMETER_CHANGE)
> > 4 - stop the server and make the wal_level back to replica
> > 5 - start the server again
> > 6 - execute archive recoveries in both cases
> > (1) by editing the postgresql.conf and
> > touching recovery.signal in the base backup from 1th step
> > (2) by making a replica with standby.signal
> > * During wal_level is replica, I enabled archive_mode in this test.
> >
> Perhaps we need the TAP test that conducts the above steps.
It really makes sense that it's better to show the procedures
about how to reproduce the flow.
Thanks for your advice.
Best,
Takamichi Osumi
Hello, Sergei
> It is possible only with manually configured hot_standby = off, right?
> We have ERROR in hot standby mode just below in
> CheckRequiredParameterValues.
Yes, you are right. I turned off the hot_standby in the test in the previous e-mail.
I'm sorry that I missed writing this tip of information.
Best,
Takamichi Osumi
Hi
On Thursday, November 26, 2020 4:29 PM
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> At Thu, 26 Nov 2020 07:18:39 +0000, "osumi.takamichi@fujitsu.com"
> <osumi.takamichi@fujitsu.com> wrote in
> > The attached patch is intended to prevent a scenario that archive
> > recovery hits WALs which come from wal_level=minimal and the server
> > continues to work, which was discussed in the thread of [1].
> > The motivation is to protect that user ends up with both getting
> > replica that could miss data and getting the server to miss data in targeted
> recovery mode.
> >
> > About how to modify this, we reached the consensus in the thread.
> > It is by changing the ereport's level from WARNING to FATAL in
> CheckRequiredParameterValues().
> >
> > In order to test this fix, what I did is
> > 1 - get a base backup during wal_level is replica
> > 2 - stop the server and change the wal_level from replica to minimal
> > 3 - restart the server(to generate XLOG_PARAMETER_CHANGE)
> > 4 - stop the server and make the wal_level back to replica
> > 5 - start the server again
> > 6 - execute archive recoveries in both cases
> > (1) by editing the postgresql.conf and
> > touching recovery.signal in the base backup from 1th step
> > (2) by making a replica with standby.signal
> > * During wal_level is replica, I enabled archive_mode in this test.
> >
> > First of all, I confirmed the server started up without this patch.
> > After applying this safeguard patch, I checked that the server cannot
> > start up any more in the scenario case.
> > I checked the log and got the result below with this patch.
> >
> > 2020-11-26 06:49:46.003 UTC [19715] FATAL: WAL was generated with
> > wal_level=minimal, data may be missing
> > 2020-11-26 06:49:46.003 UTC [19715] HINT: This happens if you
> temporarily set wal_level=minimal without taking a new base backup.
> >
> > Lastly, this should be backpatched.
> > Any comments ?
>
> Perhaps we need the TAP test that conducts the above steps.
I added the TAP tests to reproduce and share the result,
using the case of 6-(1) described above.
Here, I created a new file for it because the purposes of other files in
src/recovery didn't match the purpose of my TAP tests perfectly.
If you are dubious about this idea, please have a look at the comments
in each file.
When the attached patch is applied,
my TAP tests are executed like other ones like below.
t/018_wal_optimize.pl ................ ok
t/019_replslot_limit.pl .............. ok
t/020_archive_status.pl .............. ok
t/021_row_visibility.pl .............. ok
t/022_archive_recovery.pl ............ ok
All tests successful.
Also, I confirmed that there's no regression by make check-world.
Any comments ?
Best,
Takamichi Osumi
Вложения
On Tue, 2020-12-08 at 03:08 +0000, osumi.takamichi@fujitsu.com wrote: > On Thursday, November 26, 2020 4:29 PM > Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 26 Nov 2020 07:18:39 +0000, "osumi.takamichi@fujitsu.com" > > <osumi.takamichi@fujitsu.com> wrote in > > > The attached patch is intended to prevent a scenario that archive > > > recovery hits WALs which come from wal_level=minimal and the server > > > continues to work, which was discussed in the thread of [1]. > > > > Perhaps we need the TAP test that conducts the above steps. > > I added the TAP tests to reproduce and share the result, > using the case of 6-(1) described above. > Here, I created a new file for it because the purposes of other files in > src/recovery didn't match the purpose of my TAP tests perfectly. > If you are dubious about this idea, please have a look at the comments > in each file. > > When the attached patch is applied, > my TAP tests are executed like other ones like below. > > t/018_wal_optimize.pl ................ ok > t/019_replslot_limit.pl .............. ok > t/020_archive_status.pl .............. ok > t/021_row_visibility.pl .............. ok > t/022_archive_recovery.pl ............ ok > All tests successful. > > Also, I confirmed that there's no regression by make check-world. > Any comments ? The patch applies and passes regression tests, as well as the new TAP test. I think this should be backpatched, since it fixes a bug. I am not quite happy with the message: FATAL: WAL was generated with wal_level=minimal, data may be missing HINT: This happens if you temporarily set wal_level=minimal without taking a new base backup. This sounds too harmless to me and doesn't give the user a clue what would be the best way to proceed. Suggestion: FATAL: WAL was generated with wal_level=minimal, cannot continue recovering DETAIL: This happens if you temporarily set wal_level=minimal on the primary server. HINT: Create a new standby from a new base backup after setting wal_level=replica. Yours, Laurenz Albe
Hi, Laurenz
On Friday, January 15, 2021 12:56 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Tue, 2020-12-08 at 03:08 +0000, osumi.takamichi@fujitsu.com wrote:
> > On Thursday, November 26, 2020 4:29 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > > At Thu, 26 Nov 2020 07:18:39 +0000, "osumi.takamichi@fujitsu.com"
> > > <osumi.takamichi@fujitsu.com> wrote in
> > > > The attached patch is intended to prevent a scenario that archive
> > > > recovery hits WALs which come from wal_level=minimal and the
> > > > server continues to work, which was discussed in the thread of [1].
> > >
> > > Perhaps we need the TAP test that conducts the above steps.
> >
> > I added the TAP tests to reproduce and share the result, using the
> > case of 6-(1) described above.
> > Here, I created a new file for it because the purposes of other files
> > in src/recovery didn't match the purpose of my TAP tests perfectly.
> > If you are dubious about this idea, please have a look at the comments
> > in each file.
> >
> > When the attached patch is applied,
> > my TAP tests are executed like other ones like below.
> >
> > t/018_wal_optimize.pl ................ ok t/019_replslot_limit.pl
> > .............. ok t/020_archive_status.pl .............. ok
> > t/021_row_visibility.pl .............. ok t/022_archive_recovery.pl
> > ............ ok All tests successful.
> >
> > Also, I confirmed that there's no regression by make check-world.
> > Any comments ?
>
> The patch applies and passes regression tests, as well as the new TAP test.
Thank you for checking.
> I think this should be backpatched, since it fixes a bug.
Agreed.
> I am not quite happy with the message:
>
> FATAL: WAL was generated with wal_level=minimal, data may be missing
> HINT: This happens if you temporarily set wal_level=minimal without taking a
> new base backup.
>
> This sounds too harmless to me and doesn't give the user a clue what would be
> the best way to proceed.
>
> Suggestion:
>
> FATAL: WAL was generated with wal_level=minimal, cannot continue
> recovering
Adopted.
> DETAIL: This happens if you temporarily set wal_level=minimal on the primary
> server.
> HINT: Create a new standby from a new base backup after setting
> wal_level=replica.
Thanks for your suggestion.
I noticed that this message should cover both archive recovery modes,
which means in recovery mode and standby mode. Then, I combined your
suggestion above with this point of view. Have a look at the updated patch.
I also enriched the new tap tests to show this perspective.
Best Regards,
Takamichi Osumi
Вложения
On Mon, 2021-01-18 at 07:34 +0000, osumi.takamichi@fujitsu.com wrote: > I noticed that this message should cover both archive recovery modes, > which means in recovery mode and standby mode. Then, I combined your > suggestion above with this point of view. Have a look at the updated patch. > I also enriched the new tap tests to show this perspective. Looks good, thanks. I'll mark this patch as "ready for committer". Yours, Laurenz Albe
On 2021/01/20 1:05, Laurenz Albe wrote:
> On Mon, 2021-01-18 at 07:34 +0000, osumi.takamichi@fujitsu.com wrote:
>> I noticed that this message should cover both archive recovery modes,
>> which means in recovery mode and standby mode. Then, I combined your
>> suggestion above with this point of view. Have a look at the updated patch.
>> I also enriched the new tap tests to show this perspective.
>
> Looks good, thanks.
>
> I'll mark this patch as "ready for committer".
+ errhint("Run recovery again from a new base backup taken after setting wal_level higher than
minimal")));
Isn't it impossible to do this in normal archive recovery case? In that case,
since the server crashed and the database got corrupted, probably
we cannot take a new base backup.
Originally even when users accidentally set wal_level to minimal, they could
start the server from the backup by disabling hot_standby and salvage the data.
But with the patch, we lost the way to do that. Right? I was wondering that
WARNING was used intentionally there for that case.
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
primaryserver"),
errhint("Either set wal_level to \"replica\" on the primary, or turn off hot_standby here.")));
With the patch, we never reach the above code?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Wed, 2021-01-20 at 13:10 +0900, Fujii Masao wrote:
> + errhint("Run recovery again from a new base backup taken after setting wal_level
higherthan minimal")));
>
> Isn't it impossible to do this in normal archive recovery case? In that case,
> since the server crashed and the database got corrupted, probably
> we cannot take a new base backup.
>
> Originally even when users accidentally set wal_level to minimal, they could
> start the server from the backup by disabling hot_standby and salvage the data.
> But with the patch, we lost the way to do that. Right? I was wondering that
> WARNING was used intentionally there for that case.
I would argue that if you set your "wal_level" to minimal, do some work,
reset it to "replica" and recover past that, two things could happen:
1. Since "archive_mode" was off, you are missing some WAL segments and
cannot recover past that point anyway.
2. You are missing some relevant WAL records, and your recovered
database is corrupted. This is very likely, because you probably
switched to "minimal" with the intention to generate less WAL.
Now I see your point that a corrupted database may be better than no
database at all, but wouldn't you agree that a warning in the log
(that nobody reads) is too little information?
Normally we don't take such a relaxed attitude towards data corruption.
Perhaps there could be a GUC "recovery_allow_data_corruption" to
override this check, but I'd say that a warning is too little.
> 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_standbyhere.")));
>
> With the patch, we never reach the above code?
Right, that would have to go. I didn't notice that.
Yours,
Laurenz Albe
Hi
On Wed, Jan 20, 2021 9:03 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Wed, 2021-01-20 at 13:10 +0900, Fujii Masao wrote:
> > + errhint("Run recovery again from a
> > + new base backup taken after setting wal_level higher than
> > + minimal")));
> >
> > Isn't it impossible to do this in normal archive recovery case? In
> > that case, since the server crashed and the database got corrupted,
> > probably we cannot take a new base backup.
> >
> > Originally even when users accidentally set wal_level to minimal, they
> > could start the server from the backup by disabling hot_standby and salvage
> the data.
> > But with the patch, we lost the way to do that. Right? I was wondering
> > that WARNING was used intentionally there for that case.
OK. I understand that this WARNING is necessary to give user a chance
to start up the server again and salvage his/her data,
when hot_standby=off and the server goes through a period of wal_level=minimal
during an archive recovery.
> I would argue that if you set your "wal_level" to minimal, do some work, reset it
> to "replica" and recover past that, two things could happen:
>
> 1. Since "archive_mode" was off, you are missing some WAL segments and
> cannot recover past that point anyway.
>
> 2. You are missing some relevant WAL records, and your recovered
> database is corrupted. This is very likely, because you probably
> switched to "minimal" with the intention to generate less WAL.
>
> Now I see your point that a corrupted database may be better than no database
> at all, but wouldn't you agree that a warning in the log (that nobody reads) is
> too little information?
>
> Normally we don't take such a relaxed attitude towards data corruption.
Yeah, we thought we needed stronger protection for that reason.
> Perhaps there could be a GUC "recovery_allow_data_corruption" to override this
> check, but I'd say that a warning is too little.
>
> > 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.")));
> >
> > With the patch, we never reach the above code?
>
> Right, that would have to go. I didn't notice that.
Adding a condition to check if "recovery_allow_data_corruption" is 'on' around the end of
CheckRequiredParameterValues() sounds safer for me too, although
implementing a new GUC parameter sounds bigger than what I expected at first.
The default of the value should be 'off' to protect users from getting the corrupted server.
Does everyone agree with this direction ?
Best Regards,
Takamichi Osumi
On Thu, 2021-01-21 at 11:49 +0000, osumi.takamichi@fujitsu.com wrote: > Adding a condition to check if "recovery_allow_data_corruption" is 'on' around the end of > CheckRequiredParameterValues() sounds safer for me too, although > implementing a new GUC parameter sounds bigger than what I expected at first. > The default of the value should be 'off' to protect users from getting the corrupted server. > Does everyone agree with this direction ? I'd say that adding such a GUC is material for another patch, if we want it at all. I think it is very unlikely that people will switch from "wal_level=replica" to "minimal" and back very soon afterwards and also try to recover past such a switch, which probably explains why nobody has complained about data corruption generated that way. To get the server to start with "wal_level=minimal", you must set "archive_mode=off" and "max_wal_senders=0", and few people will do that and still expect recovery to work. My vote is that we should not have a GUC for such an unlikely event, and that stopping recovery is good enough. Yours, Laurenz Albe
Hi, Laurenz
On Thursday, January 21, 2021 9:51 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Thu, 2021-01-21 at 11:49 +0000, osumi.takamichi@fujitsu.com wrote:
> > Adding a condition to check if "recovery_allow_data_corruption" is
> > 'on' around the end of
> > CheckRequiredParameterValues() sounds safer for me too, although
> > implementing a new GUC parameter sounds bigger than what I expected at
> first.
> > The default of the value should be 'off' to protect users from getting the
> corrupted server.
> > Does everyone agree with this direction ?
>
> I'd say that adding such a GUC is material for another patch, if we want it at all.
OK. You meant another different patch.
> I think it is very unlikely that people will switch from "wal_level=replica" to
> "minimal" and back very soon afterwards and also try to recover past such a
> switch, which probably explains why nobody has complained about data
> corruption generated that way. To get the server to start with
> "wal_level=minimal", you must set "archive_mode=off" and
> "max_wal_senders=0", and few people will do that and still expect recovery to
> work.
Yeah, the possibility is low of course.
> My vote is that we should not have a GUC for such an unlikely event, and that
> stopping recovery is good enough.
OK. IIUC, my current patch for this fix doesn't need to be changed or withdrawn.
Thank you for your explanation.
Best Regards,
Takamichi Osumi
On Thu, 2021-01-21 at 13:09 +0000, osumi.takamichi@fujitsu.com wrote: > > My vote is that we should not have a GUC for such an unlikely event, and that > > stopping recovery is good enough. > > OK. IIUC, my current patch for this fix doesn't need to be changed or withdrawn. > Thank you for your explanation. Well, that's just my opinion. Fujii Masao seemed to disagree with the patch, and his voice carries weight. Yours, Laurenz Albe
On Thu, 2021-01-21 at 15:30 +0100, I wrote: > On Thu, 2021-01-21 at 13:09 +0000, osumi.takamichi@fujitsu.com wrote: > > > > My vote is that we should not have a GUC for such an unlikely event, and that > > > stopping recovery is good enough. > > OK. IIUC, my current patch for this fix doesn't need to be changed or withdrawn. > > Thank you for your explanation. > > Well, that's just my opinion. > > Fujii Masao seemed to disagree with the patch, and his voice carries weight. I think you should pst another patch where the second, now superfluous, error message is removed. Yours, Laurenz Albe
Hi
On Monday, January 25, 2021 5:13 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> On Thu, 2021-01-21 at 15:30 +0100, I wrote:
> > On Thu, 2021-01-21 at 13:09 +0000, osumi.takamichi@fujitsu.com wrote:
> >
> > > > My vote is that we should not have a GUC for such an unlikely
> > > > event, and that stopping recovery is good enough.
> > > OK. IIUC, my current patch for this fix doesn't need to be changed or
> withdrawn.
> > > Thank you for your explanation.
> >
> > Well, that's just my opinion.
> >
> > Fujii Masao seemed to disagree with the patch, and his voice carries weight.
>
> 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.
Best Regards,
Takamichi Osumi
Вложения
On Mon, 2021-01-25 at 08:19 +0000, osumi.takamichi@fujitsu.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. Yours, Laurenz Albe
On 1/25/21 3:55 AM, Laurenz Albe wrote: > On Mon, 2021-01-25 at 08:19 +0000, osumi.takamichi@fujitsu.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? Regards, -- -David david@pgmasters.net [1] https://www.postgresql.org/message-id/OSBPR01MB48887EFFCA39FA9B1DBAFB0FEDBD0%40OSBPR01MB4888.jpnprd01.prod.outlook.com
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.takamichi@fujitsu.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. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
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.takamichi@fujitsu.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@pgmasters.net
[1]
https://www.postgresql.org/message-id/E1iu6D8-0000tK-Cm%40gemulon.postgresql.org
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, osumi.takamichi@fujitsu.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
currentlya 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
beingintroduced 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.
Itjust 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
primaryserver"),
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
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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, osumi.takamichi@fujitsu.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.
>
> 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
At Wed, 31 Mar 2021 14:23:26 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> 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
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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, osumi.takamichi@fujitsu.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.
>
> 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
afterrestart.
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
At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> 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
afterrestart.
>
> 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
Hi,
On Wednesday, March 31, 2021 3:06 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote in
> > At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao
> > <masao.fujii@oss.nttdata.com> 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
mismatchfor 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@postgresql.org/msg323147.html
Best Regards,
Takamichi Osumi
Вложения
On 2021/04/01 12:45, osumi.takamichi@fujitsu.com 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
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
eventhough 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
On Thursday, April 1, 2021 5:25 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/04/01 12:45, osumi.takamichi@fujitsu.com 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
Вложения
On Friday, April 2, 2021 11:49 PM Laurenz Albe <laurenz.albe@cybertec.at> 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
On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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
Вложения
From: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> > 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'tseem like this failure is due to your patch. Regards Takayuki Tsunakawa
At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>
>
> On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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
causedata 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
At Mon, 5 Apr 2021 07:04:09 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in > From: osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> > > 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'tseem 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
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com> > 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 thecase, 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 makeinstallcheck, or will build a new local installation tree from current sources if you say make check. In either casethey will initialize a local instance (data directory) and transiently run a server in it. Some of these tests run morethan one server." "It's important to realize that the TAP tests will start test server(s) even when you say make installcheck; this is unlikethe traditional non-TAP testing infrastructure, which expects to use an already-running test server in that case. SomePostgreSQL subdirectories contain both traditional-style and TAP-style tests, meaning that make installcheck will producea mix of results from temporary servers and the already-running test server." Regards Takayuki Tsunakawa
On 2021/04/05 16:13, Kyotaro Horiguchi wrote:
> At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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
causedata 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
On Mon Apr 5, 2021 12:35 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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
On Monday, April 5, 2021 9:16 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/04/05 16:13, Kyotaro Horiguchi wrote:
> > At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote in
> >>
> >>
> >> On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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
On 4/4/21 11:34 PM, Fujii Masao wrote: > > On 2021/04/04 11:58, osumi.takamichi@fujitsu.com 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 david@pgmasters.net
On Monday, April 5, 2021 11:49 PM osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com>
> On Mon Apr 5, 2021 12:35 PM Fujii Masao <masao.fujii@oss.nttdata.com>
> wrote:
> > >>> 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.
I really apologie that this OSS HEAD reproduced that stderr with success of RT.
I executed check-world in parallel with -j option so
the reason should be what Tsunakawa-san told us.
Its probability is pretty low.
I'm so sorry for making noises loudly.
Therefore, I don't have any concern left.
Best Regards,
Takamichi Osumi
On Tuesday, April 6, 2021 8:32 AM Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com>
> On Monday, April 5, 2021 11:49 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com>
> > On Mon Apr 5, 2021 12:35 PM Fujii Masao <masao.fujii@oss.nttdata.com>
> > wrote:
> > > >>> 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.
> I really apologie that this OSS HEAD reproduced that stderr with success of
> RT.
> I executed check-world in parallel with -j option so the reason should be what
> Tsunakawa-san told us.
> Its probability is pretty low.
> I'm so sorry for making noises loudly.
> Therefore, I don't have any concern left.
This is *not* due to the patch but for future analysis.
The phenomena happens with a very little possibility, and in other case,
with --enable-coverage and make check-world causes an error like below.
I used gcc 8.
# Failed test 'pg_ctl start: no stderr'
# at t/001_start_stop.pl line 48.
# got: 'profiling:/home/(path/to/oss/head)/src/backend/utils/adt/regproc.gcda:Merge mismatch for function 24
# '
# expected: ''
# Looks like you failed 1 test of 24.
make[2]: *** [Makefile:50: check] Error 1
make[1]: *** [Makefile:43: check-pg_ctl-recurse] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [GNUmakefile:71: check-world-src/bin-recurse] Error 2
make: *** Waiting for unfinished jobs....
The steps I used are
$ git clone and cd to OSS HEAD
$ ./configure --enable-coverage --enable-cassert --enable-debug --enable-tap-tests --with-icu CFLAGS=-O0
--prefix=/where/to/put/binary
$ make -j4 2> make.log
$ make check-world -j4 2> make_check_world.log
Best Regards,
Takamichi Osumi
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
Вложения
On Tuesday, April 6, 2021 9:41 AM Fujii Masao <masao.fujii@oss.nttdata.com>
> 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.
Thank you for updating the patch. Let's make the sentence more strict.
My suggestion for this explanation is
"In order to prevent database corruption, changing
wal_level to minimal from higher level in the middle of
WAL archiving requires careful attention. It makes any base backups
taken before the operation unavailable for archive recovery
and standby server. Also, it may lead to whole database loss when
archive recovery fails with an error for that change.
Take a new base backup immediately after making wal_level back to higher level."
Then, we can be consistent with our new hint message,
"Use a backup taken after setting wal_level to higher than minimal.".
Is it better to add something similar to "Take an offline backup when you stop the server
and change the wal_level" around the end of this part as another option for safeguard, also?
For the performance technique part, what we need to explain is same.
Another minor thing I felt we need to do might be to add double quotes to wrap minimal in errhint.
Other errhints do so when we use it in a sentence.
There is no more additional comment from me !
Best Regards,
Takamichi Osumi
At Tue, 6 Apr 2021 04:11:35 +0000, "osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com> wrote in > On Tuesday, April 6, 2021 9:41 AM Fujii Masao <masao.fujii@oss.nttdata.com> > > 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. > Thank you for updating the patch. Let's make the sentence more strict. > > My suggestion for this explanation is > "In order to prevent database corruption, changing > wal_level to minimal from higher level in the middle of > WAL archiving requires careful attention. It makes any base backups > taken before the operation unavailable for archive recovery > and standby server. Also, it may lead to whole database loss when > archive recovery fails with an error for that change. > Take a new base backup immediately after making wal_level back to higher level." The first sentense looks like somewhat nanny-ish. The database is not corrupt at the time of this error. We just lose updates after the last read segment at this point. As Fujii-san said, we can continue recoverying using crash recovery and we will reach having a corrupt database after that. About the last sentense, I prefer more flat wording, such as "You need to take a new base backup..." > Then, we can be consistent with our new hint message, > "Use a backup taken after setting wal_level to higher than minimal.". > > Is it better to add something similar to "Take an offline backup when you stop the server > and change the wal_level" around the end of this part as another option for safeguard, also? Backup policy is completely a matter of DBAs. If flipping wal_level alone highly causes unstartable corruption,,, I think it is a bug. > For the performance technique part, what we need to explain is same. Might be good, but in simpler wording. > Another minor thing I felt we need to do might be to add double quotes to wrap minimal in errhint. Since the error about hot_standby has gone, either will do for me. > Other errhints do so when we use it in a sentence. > > There is no more additional comment from me ! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Tuesday, April 6, 2021 3:24 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
> At Tue, 6 Apr 2021 04:11:35 +0000, "osumi.takamichi@fujitsu.com"
> <osumi.takamichi@fujitsu.com> wrote in
> > On Tuesday, April 6, 2021 9:41 AM Fujii Masao
> > <masao.fujii@oss.nttdata.com>
> > > 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.
> > Thank you for updating the patch. Let's make the sentence more strict.
> >
> > My suggestion for this explanation is
> > "In order to prevent database corruption, changing wal_level to
> > minimal from higher level in the middle of WAL archiving requires
> > careful attention. It makes any base backups taken before the
> > operation unavailable for archive recovery and standby server. Also,
> > it may lead to whole database loss when archive recovery fails with an
> > error for that change.
> > Take a new base backup immediately after making wal_level back to higher
> level."
>
> The first sentense looks like somewhat nanny-ish. The database is not
> corrupt at the time of this error.
Yes. Excuse me for misleading sentence.
I just wanted to write why the error was introduced,
but it was not necessary.
We should remove and fix the first part of the sentence.
> We just lose updates after the last read
> segment at this point. As Fujii-san said, we can continue recoverying using
> crash recovery and we will reach having a corrupt database after that.
OK. Thank you for explanation.
> About the last sentence, I prefer more flat wording, such as "You need to take
> a new base backup..."
Either is fine to me.
> > Then, we can be consistent with our new hint message, "Use a backup
> > taken after setting wal_level to higher than minimal.".
>
> > Is it better to add something similar to "Take an offline backup when
> > you stop the server and change the wal_level" around the end of this part as
> another option for safeguard, also?
>
> Backup policy is completely a matter of DBAs.
OK. No problem. No need to add it.
> If flipping wal_level alone
> highly causes unstartable corruption,,, I think it is a bug.
> > For the performance technique part, what we need to explain is same.
>
> Might be good, but in simpler wording.
Yeah, I agree.
> > Another minor thing I felt we need to do might be to add double quotes to
> wrap minimal in errhint.
>
> Since the error about hot_standby has gone, either will do for me.
Thanks for sharing your thoughts.
Best Regards,
Takamichi Osumi
On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote: > I just wanted to write why the error was introduced, > but it was not necessary. > We should remove and fix the first part of the sentence. So the consensus is almost the same as the latest patch? If they are not so different, I'm thinking to push the latest version at first. Then we can improve the docs if required. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 4/6/21 7:13 AM, Fujii Masao wrote: > > On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote: >> I just wanted to write why the error was introduced, >> but it was not necessary. >> We should remove and fix the first part of the sentence. > > So the consensus is almost the same as the latest patch? > If they are not so different, I'm thinking to push the latest version at > first. > Then we can improve the docs if required. +1 -- -David david@pgmasters.net
On Tuesday, April 6, 2021 8:44 PM David Steele <david@pgmasters.net> wrote:
> On 4/6/21 7:13 AM, Fujii Masao wrote:
> >
> > On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote:
> >> I just wanted to write why the error was introduced, but it was not
> >> necessary.
> >> We should remove and fix the first part of the sentence.
> >
> > So the consensus is almost the same as the latest patch?
> > If they are not so different, I'm thinking to push the latest version
> > at first.
> > Then we can improve the docs if required.
>
> +1
Yes, please. What I suggested is almost same as your idea.
Thank you for your confirmation.
Best Regards,
Takamichi Osumi
On 2021/04/06 20:44, David Steele wrote: > On 4/6/21 7:13 AM, Fujii Masao wrote: >> >> On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote: >>> I just wanted to write why the error was introduced, >>> but it was not necessary. >>> We should remove and fix the first part of the sentence. >> >> So the consensus is almost the same as the latest patch? >> If they are not so different, I'm thinking to push the latest version atfirst. >> Then we can improve the docs if required. > > +1 Thanks! So I pushed the patch. On 2021/04/06 20:48, osumi.takamichi@fujitsu.com wrote: > Yes, please. What I suggested is almost same as your idea. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/04/06 23:01, Fujii Masao wrote:
>
>
> On 2021/04/06 20:44, David Steele wrote:
>> On 4/6/21 7:13 AM, Fujii Masao wrote:
>>>
>>> On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote:
>>>> I just wanted to write why the error was introduced,
>>>> but it was not necessary.
>>>> We should remove and fix the first part of the sentence.
>>>
>>> So the consensus is almost the same as the latest patch?
>>> If they are not so different, I'm thinking to push the latest version atfirst.
>>> Then we can improve the docs if required.
>>
>> +1
>
> Thanks! So I pushed the patch.
The buildfarm members "drongo" and "fairywren" reported that the regression test (024_archive_recovery.pl) commit
9de9294b0cadded failed with the following error. ISTM the cause of this failure is that the test calls $node->init()
without"allows_streaming => 1" and which doesn't add pg_hba.conf entry for TCP/IP connection from pg_basebackup. So I
thinkthat the attached patch needs to be applied.
pg_basebackup: error: connection to server at "127.0.0.1", port 52316 failed: FATAL: no pg_hba.conf entry for
replicationconnection from host "127.0.0.1", user "pgrunner", no encryption
Bail out! system pg_basebackup failed
I guess that only "drongo" and "fairywren" reported this issue because they are running on Windows and pg_basebackup
usesTCP/IP connection. OTOH I guess that other buildfarm members running on non-Windows use Unix-domain for
pg_basebackupand pg_hba.conf entry for that exists by default. So ISTM they reported no such issue.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Вложения
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> Thanks! So I pushed the patch.
Seems like the test case added by this commit is not
working on Windows.
regards, tom lane
On 2021/04/07 3:03, Fujii Masao wrote: > > > On 2021/04/06 23:01, Fujii Masao wrote: >> >> >> On 2021/04/06 20:44, David Steele wrote: >>> On 4/6/21 7:13 AM, Fujii Masao wrote: >>>> >>>> On 2021/04/06 15:59, osumi.takamichi@fujitsu.com wrote: >>>>> I just wanted to write why the error was introduced, >>>>> but it was not necessary. >>>>> We should remove and fix the first part of the sentence. >>>> >>>> So the consensus is almost the same as the latest patch? >>>> If they are not so different, I'm thinking to push the latest version atfirst. >>>> Then we can improve the docs if required. >>> >>> +1 >> >> Thanks! So I pushed the patch. > > The buildfarm members "drongo" and "fairywren" reported that the regression test (024_archive_recovery.pl) commit 9de9294b0cadded failed with the following error. ISTM the cause of this failure is that the test calls $node->init() without"allows_streaming => 1" and which doesn't add pg_hba.conf entry for TCP/IP connection from pg_basebackup. So I thinkthat the attached patch needs to be applied. Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2021/04/07 5:01, Tom Lane wrote: > Fujii Masao <masao.fujii@oss.nttdata.com> writes: >> Thanks! So I pushed the patch. > > Seems like the test case added by this commit is not > working on Windows. Thanks for the report! My analysis is [1], and I pushed the proposed patch. [1] https://postgr.es/m/3cc3909d-f779-7a74-c201-f1f7f62c7497@oss.nttdata.com Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Wednesday, April 7, 2021 7:50 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > On 2021/04/07 5:01, Tom Lane wrote: > > Fujii Masao <masao.fujii@oss.nttdata.com> writes: > >> Thanks! So I pushed the patch. > > > > Seems like the test case added by this commit is not working on > > Windows. > > Thanks for the report! My analysis is [1], and I pushed the proposed patch. > > [1] > https://postgr.es/m/3cc3909d-f779-7a74-c201-f1f7f62c7497@oss.nttdata.co > m Fujii-san, Tom, thank you so much for your quick analysis and modification. I appreciate those. Best Regards, Takamichi Osumi