From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
Cc: michael@paquier.xyz
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Date: Fri, 3 Apr 2020 18:26:25 +0200
Message-ID: <20200403182625.0fccc6fd@firost> (raw)
In-Reply-To: <3d543ef2-ae02-5e59-3bfe-b79d00dab360@oss.nttdata.com>
References: <20200402.130443.1791362864704140391.horikyota.ntt@gmail.com>
<20200402.130734.613319908534820379.horikyota.ntt@gmail.com>
<03900fc5-fff4-109a-fe69-83de2c167929@oss.nttdata.com>
<20200402.162323.770257425358491539.horikyota.ntt@gmail.com>
<ca964b3a-61a0-902e-c7b3-3abbc01a921f@oss.nttdata.com>
<20200402154915.4984cff2@firost>
<3d543ef2-ae02-5e59-3bfe-b79d00dab360@oss.nttdata.com>
On Thu, 2 Apr 2020 23:55:46 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2020/04/02 22:49, Jehan-Guillaume de Rorthais wrote:
>> On Thu, 2 Apr 2020 19:38:59 +0900
>> Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
[...]
>>> That is, WAL files with .ready files are removed when either
>>> archive_mode!=always in standby mode or archive_mode=off.
>>
>> sounds fine to me.
To some extends, it appears to me this sentence was relatively close to my
previous patch, as far as you exclude IN_CRASH_RECOVERY from the shortcut:
XLogArchiveCheckDone(const char *xlog)
{
[...]
if ( (inRecoveryState != IN_CRASH_RECOVERY) && (
(inRecoveryState == NOT_IN_RECOVERY && !XLogArchivingActive()) ||
(inRecoveryState == IN_ARCHIVE_RECOVERY && !XLogArchivingAlways())))
return true;
Which means that only .done cleanup would occurs during CRASH_RECOVERY
and .ready files might be created if no .done exists. No matter the futur status
of the cluster: primary or standby. Normal shortcut will apply during first
checkpoint after the crash recovery step.
This should handle the case where a backup without backup_label (taken from a
snapshot or after a shutdown with immediate) is restored to build a standby.
Please, find in attachment a third version of my patch
"0001-v3-Fix-WAL-retention-during-production-crash-recovery.patch".
"0002-v1-Add-test-on-non-archived-WAL-during-crash-recovery.patch" is left
untouched. But I'm considering adding some more tests relative to this
discussion.
> BTW, now I'm thinking that the flag in shmem should be updated when
> the startup process sets StandbyModeRequested to true at the beginning
> of the recovery. That is,
>
> - Add something like SharedStandbyModeRequested into XLogCtl. This field
> should be initialized with false;
> - Set XLogCtl->SharedStandbyModeRequested to true when the startup
> process detects the standby.signal file and sets the local variable
> StandbyModeRequested to true.
> - Make XLogArchiveCheckDone() use XLogCtl->SharedStandbyModeRequested
> to know whether the server is in standby mode or not.
>
> Thought?
I try to avoid a new flag in memory with the proposal in attachment of this
email. It seems to me various combinaisons of booleans with subtle differences
around the same subject makes it a bit trappy and complicated to understand.
If my proposal is rejected, I'll be happy to volunteer to add
XLogCtl->SharedStandbyModeRequested though. It seems like a simple enough fix
as well.
Regards,
Attachments:download all (2 files, .tar.gz)0001-v3-Fix-WAL-retention-during-production-crash-recovery.patch (text/x-patch, 5.3K) download
From 834235add60df8203c243fa30e78cfa62619076b Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Wed, 1 Apr 2020 16:36:06 +0200
Subject: [PATCH 1/2] Fix WAL retention during production crash recovery
During crash recovery of a production cluster with archive_mode=on,
XLogArchiveCheckDone() was considering the cluster as inRecovery
without archive_mode=always. Because of this non-arcived WAL and
related .ready files were recycled or removed.
---
src/backend/access/transam/xlog.c | 25 +++++++++++++++++-------
src/backend/access/transam/xlogarchive.c | 7 ++++---
src/include/access/xlog.h | 9 +++++++++
3 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 977d448f50..55d06a8704 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -219,8 +219,8 @@ static TimeLineID receiveTLI = 0;
static bool lastFullPageWrites;
/*
- * Local copy of SharedRecoveryInProgress variable. True actually means "not- * known, need to check the shared state".+ * This is false when SharedRecoveryInProgress is not IN_ARCHIVE_RECOVERY.+ * True actually means "not known, need to check the shared state".
*/
static bool LocalRecoveryInProgress = true;
@@ -653,10 +653,10 @@ typedef struct XLogCtlData
TimeLineID PrevTimeLineID;
/*
- * SharedRecoveryInProgress indicates if we're still in crash or archive+ * SharedRecoveryInProgress indicates if we're either in crash or archive
* recovery. Protected by info_lck.
*/
- bool SharedRecoveryInProgress;+ RecoveryState SharedRecoveryInProgress;
/*
* SharedHotStandbyActive indicates if we allow hot standby queries to be
@@ -5131,7 +5131,7 @@ XLOGShmemInit(void)
* in additional info.)
*/
XLogCtl->XLogCacheBlck = XLOGbuffers - 1;
- XLogCtl->SharedRecoveryInProgress = true;+ XLogCtl->SharedRecoveryInProgress = IN_ARCHIVE_RECOVERY;
XLogCtl->SharedHotStandbyActive = false;
XLogCtl->SharedPromoteIsTriggered = false;
XLogCtl->WalWriterSleeping = false;
@@ -6901,6 +6901,9 @@ StartupXLOG(void)
ControlFile->checkPointCopy.ThisTimeLineID,
recoveryTargetTLI)));
ControlFile->state = DB_IN_CRASH_RECOVERY;
+ SpinLockAcquire(&XLogCtl->info_lck);+ XLogCtl->SharedRecoveryInProgress = IN_CRASH_RECOVERY;+ SpinLockRelease(&XLogCtl->info_lck);
}
ControlFile->checkPoint = checkPointLoc;
ControlFile->checkPointCopy = checkPoint;
@@ -7928,7 +7931,7 @@ StartupXLOG(void)
ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->SharedRecoveryInProgress = false;+ XLogCtl->SharedRecoveryInProgress = NOT_IN_RECOVERY;
SpinLockRelease(&XLogCtl->info_lck);
UpdateControlFile();
@@ -8074,7 +8077,7 @@ RecoveryInProgress(void)
*/
volatile XLogCtlData *xlogctl = XLogCtl;
- LocalRecoveryInProgress = xlogctl->SharedRecoveryInProgress;+ LocalRecoveryInProgress = (xlogctl->SharedRecoveryInProgress != NOT_IN_RECOVERY);
/*
* Initialize TimeLineID and RedoRecPtr when we discover that recovery
@@ -8103,6 +8106,14 @@ RecoveryInProgress(void)
}
}
+RecoveryState+GetRecoveryState(void)+{+ volatile XLogCtlData *xlogctl = XLogCtl;++ return xlogctl->SharedRecoveryInProgress;+}+
/*
* Is HotStandby active yet? This is only important in special backends
* since normal backends won't ever be able to connect until this returns
diff --git a/src/backend/access/transam/xlogarchive.c b/src/backend/access/transam/xlogarchive.cindex d62c12310a..25666cdbcf 100644--- a/src/backend/access/transam/xlogarchive.c+++ b/src/backend/access/transam/xlogarchive.c@@ -572,7 +572,7 @@ XLogArchiveCheckDone(const char *xlog)
{
char archiveStatusPath[MAXPGPATH];
struct stat stat_buf;
- bool inRecovery = RecoveryInProgress();+ RecoveryState inRecoveryState = GetRecoveryState();
/*
* The file is always deletable if archive_mode is "off". On standbys
@@ -580,8 +580,9 @@ XLogArchiveCheckDone(const char *xlog)
* "always". On a primary, archiving is enabled if archive_mode is "on"
* or "always".
*/
- if (!((XLogArchivingActive() && !inRecovery) ||- (XLogArchivingAlways() && inRecovery)))+ if ( (inRecoveryState != IN_CRASH_RECOVERY) && (+ (inRecoveryState == NOT_IN_RECOVERY && !XLogArchivingActive()) &&+ (inRecoveryState == IN_ARCHIVE_RECOVERY && !XLogArchivingAlways())))
return true;
/* First check for .done --- this means archiver is done with it */
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.hindex 9ec7b31cce..d8b08d6d17 100644--- a/src/include/access/xlog.h+++ b/src/include/access/xlog.h@@ -165,6 +165,14 @@ typedef enum WalLevel
WAL_LEVEL_LOGICAL
} WalLevel;
+/* Recovery state */+typedef enum RecoveryState+{+ NOT_IN_RECOVERY = 0,+ IN_CRASH_RECOVERY,+ IN_ARCHIVE_RECOVERY+} RecoveryState;+
extern PGDLLIMPORT int wal_level;
/* Is WAL archiving enabled (always or only while server is running normally)? */
@@ -278,6 +286,7 @@ extern const char *xlog_identify(uint8 info);
extern void issue_xlog_fsync(int fd, XLogSegNo segno);
extern bool RecoveryInProgress(void);
+extern RecoveryState GetRecoveryState(void);
extern bool HotStandbyActive(void);
extern bool HotStandbyActiveInReplay(void);
extern bool XLogInsertAllowed(void);
--
2.20.1
0002-v1-Add-test-on-non-archived-WAL-during-crash-recovery.patch (text/x-patch, 1.5K) download
From 4ad24cd903766fc21132bc5ff4a80be5969d492f Mon Sep 17 00:00:00 2001
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
Date: Wed, 1 Apr 2020 18:12:17 +0200
Subject: [PATCH 2/2] Add test on non archived WAL during crash recovery
---
src/test/recovery/t/011_crash_recovery.pl | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/test/recovery/t/011_crash_recovery.pl b/src/test/recovery/t/011_crash_recovery.plindex 526a3481fb..ad512b881f 100644--- a/src/test/recovery/t/011_crash_recovery.pl+++ b/src/test/recovery/t/011_crash_recovery.pl@@ -15,11 +15,17 @@ if ($Config{osname} eq 'MSWin32')
}
else
{
- plan tests => 3;+ plan tests => 4;
}
my $node = get_new_node('master');
-$node->init(allows_streaming => 1);+$node->init(+ has_archiving => 1,+ allows_streaming => 1);++$node->append_conf('postgresql.conf',+ "archive_command = ''");+
$node->start;
my ($stdin, $stdout, $stderr) = ('', '', '');
@@ -43,6 +49,7 @@ $stdin .= q[
BEGIN;
CREATE TABLE mine(x integer);
SELECT txid_current();
+SELECT pg_switch_wal();
];
$tx->pump until $stdout =~ /[[:digit:]]+[\r\n]$/;
@@ -65,4 +72,9 @@ cmp_ok($node->safe_psql('postgres', 'SELECT txid_current()'),
is($node->safe_psql('postgres', qq[SELECT txid_status('$xid');]),
'aborted', 'xid is aborted after crash');
+# make WAL are still waiting to be archived+my $node_data = $node->data_dir;+ok( -f "$node_data/pg_wal/archive_status/000000010000000000000001.ready",+ "WAL still waiting to be archived");+
$tx->kill_kill;
--
2.20.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, masao.fujii@oss.nttdata.com, horikyota.ntt@gmail.com, pgsql-bugs@lists.postgresql.org, michael@paquier.xyz
Subject: Re: [BUG] non archived WAL removed during production crash recovery
In-Reply-To: <20200403182625.0fccc6fd@firost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox