From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Date: Wed, 1 Apr 2020 18:17:35 +0200
Message-ID: <20200401181735.11100908@firost> (raw)
In-Reply-To: <d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
References: <20200331172229.40ee00dc@firost>
<d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
On Wed, 1 Apr 2020 17:27:22 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
[...]
> > bool inRecovery = RecoveryInProgress();
> >
> > /*
> > * The file is always deletable if archive_mode is "off". On standbys
> > * archiving is disabled if archive_mode is "on", and enabled with
> > * "always". On a primary, archiving is enabled if archive_mode is "on"
> > * or "always".
> > */
> > if (!((XLogArchivingActive() && !inRecovery) ||
> > (XLogArchivingAlways() && inRecovery)))
> > return true;
> >
> > Please find in attachment a patch that fix this issue using the following
> > test instead:
> >
> > if (!((XLogArchivingActive() && !StandbyModeRequested) ||
> > (XLogArchivingAlways() && inRecovery)))
> > return true;
> >
> > I'm not sure if we should rely on StandbyModeRequested for the second part
> > of the test as well thought. What was the point to rely on
> > RecoveryInProgress() to get the recovery status from shared mem?
>
> Since StandbyModeRequested is the startup process-local variable,
> it basically cannot be used in XLogArchiveCheckDone() that other process
> may call.
Ok, you answered my wondering about using recovery status from shared mem. This
was obvious. Thanks for your help!
I was wondering if we could use "ControlFile->state != DB_IN_CRASH_RECOVERY".
It seems fine during crash recovery as the control file is updated before the
checkpoint, but it doesn't feel right for other code paths where the control
file might not be up-to-date on filesystem, right ?
> So another approach would be necessary... One straight idea is to
> add new shmem flag indicating whether we are in standby mode or not.
I was thinking about setting XLogCtlData->SharedRecoveryInProgress as an enum
using:
enum RecoveryState
{
NOT_IN_RECOVERY = 0
IN_CRASH_RECOVERY,
IN_ARCHIVE_RECOVERY
}
Please, find in attachment a patch implementing this.
Plus, I added a second commit to add one test in regard with this bug.
> Another is to make the startup process remove .ready file if necessary.
I'm not sure to understand this one.
Regards,
Attachments:download all (2 files, .tar.gz)0001-v2-Fix-WAL-retention-during-production-crash-recovery.patch (text/x-patch, 5.5K) download
From e5e0f7a0bf9d5f0b4097c53016fd7cc381ef551c 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 | 11 ++++++++---
src/include/access/xlog.h | 9 +++++++++
3 files changed, 35 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..0f9e10bbef 100644--- a/src/backend/access/transam/xlogarchive.c+++ b/src/backend/access/transam/xlogarchive.c@@ -572,16 +572,21 @@ XLogArchiveCheckDone(const char *xlog)
{
char archiveStatusPath[MAXPGPATH];
struct stat stat_buf;
- bool inRecovery = RecoveryInProgress();+ RecoveryState inRecoveryState = GetRecoveryState();+ ereport(DEBUG2,+ (errmsg("XLogArchivingActive(): %d, XLogArchivingAlways: %d, inRecoveryState: %d (%d, %d, %d)",+ XLogArchivingActive(), XLogArchivingAlways(),+ inRecoveryState,+ NOT_IN_RECOVERY, IN_CRASH_RECOVERY, IN_ARCHIVE_RECOVERY)));
/*
* The file is always deletable if archive_mode is "off". On standbys
* archiving is disabled if archive_mode is "on", and enabled with
* "always". On a primary, archiving is enabled if archive_mode is "on"
* or "always".
*/
- if (!((XLogArchivingActive() && !inRecovery) ||- (XLogArchivingAlways() && inRecovery)))+ if (!((XLogArchivingActive() && inRecoveryState != IN_ARCHIVE_RECOVERY) ||+ (XLogArchivingAlways() && inRecoveryState == IN_ARCHIVE_RECOVERY)))
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, michael@paquier.xyz
Subject: Re: [BUG] non archived WAL removed during production crash recovery
In-Reply-To: <20200401181735.11100908@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