RE: Stronger safeguard for archive recovery not to miss data
On Wednesday, April 7, 2021 7:50 AM Fujii Masao wrote: > On 2021/04/07 5:01, Tom Lane wrote: > > Fujii Masao 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-f1f7f62c7...@oss.nttdata.co > m Fujii-san, Tom, thank you so much for your quick analysis and modification. I appreciate those. Best Regards, Takamichi Osumi
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/07 5:01, Tom Lane wrote: Fujii Masao 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-f1f7f62c7...@oss.nttdata.com Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Stronger safeguard for archive recovery not to miss data
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.takami...@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 9de9294b0c added 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 think that the attached patch needs to be applied. Pushed. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Stronger safeguard for archive recovery not to miss data
Fujii Masao writes: > Thanks! So I pushed the patch. Seems like the test case added by this commit is not working on Windows. regards, tom lane
Re: Stronger safeguard for archive recovery not to miss data
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.takami...@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 9de9294b0c added 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 think that 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 replication connection 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 uses TCP/IP connection. OTOH I guess that other buildfarm members running on non-Windows use Unix-domain for pg_basebackup and 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 diff --git a/src/test/recovery/t/024_archive_recovery.pl b/src/test/recovery/t/024_archive_recovery.pl index c139db2ede..2d8d59454e 100644 --- a/src/test/recovery/t/024_archive_recovery.pl +++ b/src/test/recovery/t/024_archive_recovery.pl @@ -9,7 +9,7 @@ use Time::HiRes qw(usleep); # Initialize and start node with wal_level = replica and WAL archiving # enabled. my $node = get_new_node('orig'); -$node->init(has_archiving => 1); +$node->init(has_archiving => 1, allows_streaming => 1); my $replica_config = q[ wal_level = replica archive_mode = on
Re: Stronger safeguard for archive recovery not to miss data
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.takami...@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.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
On Tuesday, April 6, 2021 8:44 PM David Steele wrote: > On 4/6/21 7:13 AM, Fujii Masao wrote: > > > > On 2021/04/06 15:59, osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
On 4/6/21 7:13 AM, Fujii Masao wrote: On 2021/04/06 15:59, osumi.takami...@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 da...@pgmasters.net
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/06 15:59, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
On Tuesday, April 6, 2021 3:24 PM Kyotaro Horiguchi wrote: > At Tue, 6 Apr 2021 04:11:35 +, "osumi.takami...@fujitsu.com" > wrote in > > On Tuesday, April 6, 2021 9:41 AM Fujii Masao > > > > > On 2021/04/05 23:54, osumi.takami...@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 wal_level to > > > minimal 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
Re: Stronger safeguard for archive recovery not to miss data
At Tue, 6 Apr 2021 04:11:35 +, "osumi.takami...@fujitsu.com" wrote in > On Tuesday, April 6, 2021 9:41 AM Fujii Masao > > On 2021/04/05 23:54, osumi.takami...@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 wal_level to > > minimal 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
RE: Stronger safeguard for archive recovery not to miss data
On Tuesday, April 6, 2021 9:41 AM Fujii Masao > On 2021/04/05 23:54, osumi.takami...@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 wal_level to > minimal 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
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/05 23:49, osumi.takami...@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.takami...@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 wal_level to minimal 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 diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 0c9128a55d..effc60c07b 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2720,6 +2720,10 @@ include_dir 'conf.d' data from a base backup and the WAL logs, so replica or higher must be used to enable WAL archiving () and streaming replication. +Note that changing wal_level to +minimal makes any base backups taken before +unavailable for archive recovery and standby server, which may +lead to database loss. In logical level, the same information is logged as diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml index 70cb5a62e1..e0d3f246e9 100644 --- a/doc/src/sgml/perform.sgml +++ b/doc/src/sgml/perform.sgml @@ -1745,7 +1745,9 @@ SELECT * FROM x, y, a, b, c WHERE something AND somethingelse; to minimal, to off, and to zero. -But note that changing these settings requires a server restart. +But note that changing these settings requires a server restart, +and makes any base backups taken before unavailable for archive +recovery and standby server, which may lead to database loss. diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 6f8810e149..c1d4415a43 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6403,9 +6403,10 @@ CheckRequiredParameterValues(void) */ if (ArchiveRecoveryRequested && ControlFile->wal_level == WAL_LEVEL_MINIMAL) { - 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("Use a backup taken after setting wal_level to higher than minimal."))); } /* @@ -6414,11 +6415,6 @@ CheckRequiredParameterValues(void) */ if (ArchiveRecoveryRequested && EnableHotStandby) { - 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."))); - /* We ignore autovacuum_max_workers when we make this test. */ RecoveryRequiresIntParameter("max_connections", MaxConnections, diff --git a/src/test/recovery/t/024_archive_recovery.pl b/src/test/recovery/t/024_archive_recovery.pl new file mode 100644 index 000
RE: Stronger safeguard for archive recovery not to miss data
On Tuesday, April 6, 2021 8:32 AM Osumi, Takamichi/大墨 昂道 > On Monday, April 5, 2021 11:49 PM osumi.takami...@fujitsu.com > > > On Mon Apr 5, 2021 12:35 PM Fujii Masao > > 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
RE: Stronger safeguard for archive recovery not to miss data
On Monday, April 5, 2021 11:49 PM osumi.takami...@fujitsu.com > On Mon Apr 5, 2021 12:35 PM Fujii Masao > 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
Re: Stronger safeguard for archive recovery not to miss data
On 4/4/21 11:34 PM, Fujii Masao wrote: On 2021/04/04 11:58, osumi.takami...@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 da...@pgmasters.net
RE: Stronger safeguard for archive recovery not to miss data
On Monday, April 5, 2021 9:16 PM Fujii Masao wrote: > On 2021/04/05 16:13, Kyotaro Horiguchi wrote: > > At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao > > wrote in > >> > >> > >> On 2021/04/04 11:58, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
On Mon Apr 5, 2021 12:35 PM Fujii Masao wrote: > On 2021/04/04 11:58, osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/05 16:13, Kyotaro Horiguchi wrote: At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao wrote in On 2021/04/04 11:58, osumi.takami...@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] 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
RE: Stronger safeguard for archive recovery not to miss data
From: Kyotaro Horiguchi > I didn't see that, but found the following article. > > https://stackoverflow.com/questions/2590794/gcov-warning-merge-mismat > ch-for-summaries ... > It seems like your working directory needs some cleanup. That could very well be. It'd be safer to run "make coverage-clean" between builds. I thought otherwise that the multiple TAP tests that were simultaneously run conflicted on the .gcda file. If this is the case, we may not be able to eliminate the failure possibility. (make -J 1 circumvents this?) https://www.postgresql.org/docs/devel/install-procedure.html "If using GCC, all programs and libraries are compiled with code coverage testing instrumentation." 33.4. TAP Tests https://www.postgresql.org/docs/devel/regress-tap.html "Generically speaking, the TAP tests will test the executables in a previously-installed installation tree if you say make installcheck, or will build a new local installation tree from current sources if you say make check. In either case they will initialize a local instance (data directory) and transiently run a server in it. Some of these tests run more than one server." "It's important to realize that the TAP tests will start test server(s) even when you say make installcheck; this is unlike the traditional non-TAP testing infrastructure, which expects to use an already-running test server in that case. Some PostgreSQL subdirectories contain both traditional-style and TAP-style tests, meaning that make installcheck will produce a mix of results from temporary servers and the already-running test server." Regards Takayuki Tsunakawa
Re: Stronger safeguard for archive recovery not to miss data
At Mon, 5 Apr 2021 07:04:09 +, "tsunakawa.ta...@fujitsu.com" wrote in > From: osumi.takami...@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't seem like this failure is due to > your patch. I didn't see that, but found the following article. https://stackoverflow.com/questions/2590794/gcov-warning-merge-mismatch-for-summaries > This happens when one of the objects you're linking into an executable > changes significantly. For example it gains or loses some lines of > profilable code. It seems like your working directory needs some cleanup. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Stronger safeguard for archive recovery not to miss data
At Mon, 5 Apr 2021 12:34:53 +0900, Fujii Masao wrote in > > > On 2021/04/04 11:58, osumi.takami...@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. 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/4A0, 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
RE: Stronger safeguard for archive recovery not to miss data
From: osumi.takami...@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't seem like this failure is due to your patch. Regards Takayuki Tsunakawa
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/04 11:58, osumi.takami...@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 diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 6f8810e149..27d9ec9f91 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6403,9 +6403,11 @@ CheckRequiredParameterValues(void) */ if (ArchiveRecoveryRequested && ControlFile->wal_level == WAL_LEVEL_MINIMAL) { - 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("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."))); } /* @@ -6414,11 +6416,6 @@ CheckRequiredParameterValues(void) */ if (ArchiveRecoveryRequested && EnableHotStandby) { - 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."))); - /* We ignore autovacuum_max_workers when we make this test. */ RecoveryRequiresIntParameter("max_connections", MaxConnections, diff --git a/src/test/recovery/t/024_archive_recovery.pl b/src/test/recovery/t/024_archive_recovery.pl new file mode 100644 index 00..a00314ddc6 --- /dev/null +++ b/src/test/recovery/t/024_archive_recovery.pl @@ -0,0 +1,95 @@ +# Test for archive recovery of WAL generated with wal_level=minimal +use strict; +use warnings; +use PostgresNode; +use TestLib; +use Test::More tests => 2; +use Time::HiRes qw(usleep); + +# Initialize and start node with wal_level = replica and WAL archiving +# enabled. +my $node = get_new_node('orig'); +$node->init(has_archiving => 1); +my $replica_config = q[ +wal_level = replica +archive_mode = on +max_wal_senders = 10 +hot_standby = off +]; +$node->append_conf('pos
RE: Stronger safeguard for archive recovery not to miss data
On Friday, April 2, 2021 11:49 PM Laurenz Albe 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
RE: Stronger safeguard for archive recovery not to miss data
On Thursday, April 1, 2021 5:25 PM Fujii Masao wrote: > On 2021/04/01 12:45, osumi.takami...@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 stronger_safeguard_for_archive_recovery_v06.patch Description: stronger_safeguard_for_archive_recovery_v06.patch
Re: Stronger safeguard for archive recovery not to miss data
On Thu, 2021-04-01 at 17:25 +0900, Fujii Masao wrote: > Thanks for updating the patch! > > +errhint("Use a backup taken after setting > wal_level to higher than minimal " > +"or recover to the point in > time before wal_level becomes minimal even though it causes data loss"))); > > ISTM that "or recover to the point in time before wal_level was changed > to minimal even though it may cause data loss" sounds better. Thought? I would reduce it to "Either use a later backup, or recover to a point in time before \"wal_level\" was set to \"minimal\"." I'd say that we can leave it to the intelligence of the reader to deduce that recovering to an earlier time means more data loss. Yours, Laurenz Albe
Re: Stronger safeguard for archive recovery not to miss data
On 2021/04/01 12:45, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi, On Wednesday, March 31, 2021 3:06 PM Kyotaro Horiguchi wrote: > At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi > wrote in > > At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao > > wrote in > > > > So, I would revert all the changes in xlog.c except changing the > > > > warning to an error: > > > > -ereport(WARNING, > > > > -(errmsg("WAL was generated with > > > > wal_level=minimal, -data may be missing"), > > > > - errhint("This happens if you temporarily set > > > > -wal_level=minimal without taking a new base backup."))); > > > > +ereport(FATAL, > > > > +(errmsg("WAL was generated with > > > > wal_level=minimal, cannot continue recovering"), > > > > + errdetail("This happens if you temporarily > > > > +set > > > > wal_level=minimal on the server."), > > > > + errhint("Run recovery again from a new > base > > > > backup taken after setting wal_level higher than minimal"))); > > > I guess that users usually encounter this error because they have > > > not taken base backups yet after setting wal_level to higher than > > > minimal and have to use the old base backup for archive recovery. So > > > I'm not sure how much only this HINT is helpful for them. Isn't it > > > better to append something like "If there is no such backup, recover > > > to the point in time before wal_level is set to minimal even though > > > which cause data loss, to start the server." into HINT? > > > > I agree that the hint doesn't make sense. > > For the primary case, > > > HINT: Restart with archive recovery turned off. The past backups are no > longer usable. You need to take a new one after restart. > > > > If it's the replica case, it would be.. > > > > HINT: Start from a fresh standby created from the curent primary server. > > Start from a fresh backup... Thank you for sharing your ideas about the hint. Absolutely need to change the message. In my opinion, combining the basic idea of yours and Fujii-san's would be the best. Updated the patch and made v05. The changes I made are * rewording of errhint although this has become long ! * fix of the typo in the TAP test * modification of my past changes not to change conditions in CheckRequiredParameterValues * rename of the test file to 024_archive_recovery.pl because two files are made since the last update of this patch * pgindent is conducted to check my alignment again. By the way, when I build postgres with this patch and enable-coverage option, the results of RT becomes unstable. Does someone know the reason ? When it fails, I get stderr like below t/001_start_stop.pl .. 10/24 # Failed test 'pg_ctl start: no stderr' # at t/001_start_stop.pl line 48. # got: 'profiling:/home/k5user/new_disk/recheck/PostgreSQL-Source-Dev/src/backend/executor/execMain.gcda:Merge mismatch for function 15 # ' # expected: '' t/001_start_stop.pl .. 24/24 # Looks like you failed 1 test of 24. t/001_start_stop.pl .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/24 subtests Similar phenomena was observed in [1] and its solution seems to upgrade my gcc higher than 7. And, I did so but still get this unstable error with enable-coverage. This didn't happen when I remove enable-option and the make check-world passes. [1] - https://www.mail-archive.com/pgsql-hackers@postgresql.org/msg323147.html Best Regards, Takamichi Osumi stronger_safeguard_for_archive_recovery_v05.patch Description: stronger_safeguard_for_archive_recovery_v05.patch
Re: Stronger safeguard for archive recovery not to miss data
At Wed, 31 Mar 2021 15:03:28 +0900 (JST), Kyotaro Horiguchi wrote in > At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao > wrote in > > > So, I would revert all the changes in xlog.c except changing the > > > warning to an error: > > > - ereport(WARNING, > > > - (errmsg("WAL was generated with wal_level=minimal, > > > -data may be missing"), > > > - errhint("This happens if you temporarily set > > > -wal_level=minimal without taking a new base backup."))); > > > + ereport(FATAL, > > > + (errmsg("WAL was generated with > > > wal_level=minimal, cannot continue recovering"), > > > + errdetail("This happens if you temporarily set > > > wal_level=minimal on the server."), > > > + errhint("Run recovery again from a new base > > > backup taken after setting wal_level higher than minimal"))); > > I guess that users usually encounter this error because they have not > > taken base backups yet after setting wal_level to higher than minimal > > and have to use the old base backup for archive recovery. So I'm not > > sure > > how much only this HINT is helpful for them. Isn't it better to append > > something like "If there is no such backup, recover to the point in > > time > > before wal_level is set to minimal even though which cause data loss, > > to start the server." into HINT? > > I agree that the hint doesn't make sense. For the primary case, > HINT: Restart with archive recovery turned off. The past backups are no > longer usable. You need to take a new one after restart. > > If it's the replica case, it would be.. > > HINT: Start from a fresh standby created from the curent primary server. Start from a fresh backup... -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Stronger safeguard for archive recovery not to miss data
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao 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 +, osumi.takami...@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 after restart. If it's the replica case, it would be.. HINT: Start from a fresh standby created from the curent primary server. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Stronger safeguard for archive recovery not to miss data
At Wed, 31 Mar 2021 14:23:26 +0900 (JST), Kyotaro Horiguchi 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
Re: Stronger safeguard for archive recovery not to miss data
At Wed, 31 Mar 2021 02:11:48 +0900, Fujii Masao 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 +, osumi.takami...@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 >
Re: Stronger safeguard for archive recovery not to miss data
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 +, osumi.takami...@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. If we want to allow recovery to play past this point I think it would make more sense to have a GUC (like ignore_invalid_pages [1]) that allows recovery to proceed and emits a warning instead of fatal. Looking the patch, I see a few things: 1) Typo in the tests: This protection shold apply to recovery mode should be: This protection should apply to recovery mode 2) I don't think it should be the job of this patch to restructure the if conditions, even if it is more efficient. It just obscures the purpose of the patch. +1 So, I would revert all the changes in xlog.c except changing the warning to an error: - ereport(WARNING, - (errmsg("WAL was generated with wal_level=minimal, data may be missing"), - errhint("This happens if you temporarily set wal_level=minimal without taking a new base backup."))); + ereport(FATAL, + (errmsg("WAL was generated with wal_level=minimal, cannot continue recovering"), + errdetail("This happens if you temporarily set wal_level=minimal on the server."), + errhint("Run recovery again from a new base backup taken after setting wal_level higher than minimal"))); I guess that users usually encounter this error because they have not taken base backups yet after setting wal_level to higher than minimal and have to use the old base backup for archive recovery. So I'm not sure how much only this HINT is helpful for them. Isn't it better to append something like "If there is no such backup, recover to the point in time before wal_level is set to minimal even though which cause data loss, to start the server." into HINT? The following error will never be thrown because of the patch. So IMO the following code should be removed. if (ControlFile->wal_level < WAL_LEVEL_REPLICA) ereport(ERROR, (errmsg("hot standby is not possible because wal_level was not set to \"replica\" or higher on the primary server"), errhint("Either set wal_level to \"replica\" on the primary, or turn off hot_standby here."))); Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Stronger safeguard for archive recovery not to miss data
On 3/25/21 9:23 PM, Fujii Masao wrote: On 2021/03/25 23:21, David Steele wrote: On 1/25/21 3:55 AM, Laurenz Albe wrote: On Mon, 2021-01-25 at 08:19 +, osumi.takami...@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 da...@pgmasters.net [1] https://www.postgresql.org/message-id/E1iu6D8-tK-Cm%40gemulon.postgresql.org
Re: Stronger safeguard for archive recovery not to miss data
On 2021/03/25 23:21, David Steele wrote: On 1/25/21 3:55 AM, Laurenz Albe wrote: On Mon, 2021-01-25 at 08:19 +, osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
On 1/25/21 3:55 AM, Laurenz Albe wrote: On Mon, 2021-01-25 at 08:19 +, osumi.takami...@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 da...@pgmasters.net [1] https://www.postgresql.org/message-id/OSBPR01MB48887EFFCA39FA9B1DBAFB0FEDBD0%40OSBPR01MB4888.jpnprd01.prod.outlook.com
Re: Stronger safeguard for archive recovery not to miss data
On Mon, 2021-01-25 at 08:19 +, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi On Monday, January 25, 2021 5:13 AM Laurenz Albe wrote: > On Thu, 2021-01-21 at 15:30 +0100, I wrote: > > On Thu, 2021-01-21 at 13:09 +, osumi.takami...@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 stronger_safeguard_for_archive_recovery_v04.patch Description: stronger_safeguard_for_archive_recovery_v04.patch
Re: Stronger safeguard for archive recovery not to miss data
On Thu, 2021-01-21 at 15:30 +0100, I wrote: > On Thu, 2021-01-21 at 13:09 +, osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
On Thu, 2021-01-21 at 13:09 +, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi, Laurenz On Thursday, January 21, 2021 9:51 PM Laurenz Albe wrote: > On Thu, 2021-01-21 at 11:49 +, osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
On Thu, 2021-01-21 at 11:49 +, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi On Wed, Jan 20, 2021 9:03 PM Laurenz Albe 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
Re: Stronger safeguard for archive recovery not to miss data
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. 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_standby here."))); > > With the patch, we never reach the above code? Right, that would have to go. I didn't notice that. Yours, Laurenz Albe
Re: Stronger safeguard for archive recovery not to miss data
On 2021/01/20 1:05, Laurenz Albe wrote: On Mon, 2021-01-18 at 07:34 +, osumi.takami...@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 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? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: Stronger safeguard for archive recovery not to miss data
On Mon, 2021-01-18 at 07:34 +, osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi, Laurenz On Friday, January 15, 2021 12:56 AM Laurenz Albe wrote: > On Tue, 2020-12-08 at 03:08 +, osumi.takami...@fujitsu.com wrote: > > On Thursday, November 26, 2020 4:29 PM Kyotaro Horiguchi > > wrote: > > > At Thu, 26 Nov 2020 07:18:39 +, "osumi.takami...@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 stronger_safeguard_for_archive_recovery_v03.patch Description: stronger_safeguard_for_archive_recovery_v03.patch
Re: Stronger safeguard for archive recovery not to miss data
On Tue, 2020-12-08 at 03:08 +, osumi.takami...@fujitsu.com wrote: > On Thursday, November 26, 2020 4:29 PM > Kyotaro Horiguchi wrote: > > At Thu, 26 Nov 2020 07:18:39 +, "osumi.takami...@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
RE: Stronger safeguard for archive recovery not to miss data
Hi On Thursday, November 26, 2020 4:29 PM Kyotaro Horiguchi wrote: > At Thu, 26 Nov 2020 07:18:39 +, "osumi.takami...@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 stronger_safeguard_for_archive_recovery_v02.patch Description: stronger_safeguard_for_archive_recovery_v02.patch
RE: Stronger safeguard for archive recovery not to miss data
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
RE: Stronger safeguard for archive recovery not to miss data
Hello, Horiguchi-San On Thursday, Nov 26, 2020 4:29 PM Kyotaro Horiguchi > At Thu, 26 Nov 2020 07:18:39 +, "osumi.takami...@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
Re: Stronger safeguard for archive recovery not to miss data
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
Re: Stronger safeguard for archive recovery not to miss data
At Thu, 26 Nov 2020 07:18:39 +, "osumi.takami...@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 base backup. > > 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