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: Tue, 7 Apr 2020 17:17:36 +0200
Message-ID: <20200407171736.61906608@firost> (raw)
In-Reply-To: <4d48dbdb-e1db-a2d6-2fb2-c2a66efa7818@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>
<20200403182625.0fccc6fd@firost>
<4d48dbdb-e1db-a2d6-2fb2-c2a66efa7818@oss.nttdata.com>
On Sat, 4 Apr 2020 02:49:50 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2020/04/04 1:26, Jehan-Guillaume de Rorthais wrote:
> > 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".
>
> Thanks for updating the patch! Here are my review comments:
>
> - bool SharedRecoveryInProgress;
> + RecoveryState SharedRecoveryInProgress;
>
> Since the patch changes the meaning of this variable, the name of
> the variable should be changed? Otherwise, the current name seems
> confusing.
Indeed, fixed using SharedRecoveryState
> + SpinLockAcquire(&XLogCtl->info_lck);
> + XLogCtl->SharedRecoveryInProgress =
> IN_CRASH_RECOVERY;
> + SpinLockRelease(&XLogCtl->info_lck);
>
> As I explained upthread, this code can be reached and IN_CRASH_RECOVERY
> can be set even in standby or archive recovery. Is this right behavior that
> you're expecting?
Yes. This patch avoids archive cleanup during crash recovery altogether,
whatever the requested status for the cluster.
> Even in crash recovery case, GetRecoveryState() returns IN_ARCHIVE_RECOVERY
> until this code is reached.
I tried to stick as close as possible with "ControlFile->state" and old
XLogCtl->SharedRecoveryInProgress variables. That's why it's initialized as
IN_ARCHIVE_RECOVERY as XLogCtl->SharedRecoveryInProgress was init as true as
well.
The status itself is set during StartupXLOG when the historical code actually
tries to define and record the real state between DB_IN_ARCHIVE_RECOVERY and
DB_IN_CRASH_RECOVERY.
> Also when WAL replay is not necessary (e.g., restart of the server shutdowed
> cleanly before), GetRecoveryState() returns IN_ARCHIVE_RECOVERY because this
> code is not reached.
It is set to NOT_IN_RECOVERY at the end of StartupXLOG, in the same place we
set ControlFile->state = DB_IN_PRODUCTION. So GetRecoveryState() returns
NOT_IN_RECOVERY as soon as StartupXLOG is done when no WAL replay is necessary.
> Aren't these fragile? If XLogArchiveCheckDone() is only user of
> GetRecoveryState(), they would be ok. But if another user will appear in the
> future, it seems very easy to mistake. At least those behaviors should be
> commented in GetRecoveryState().
We certainly can set SharedRecoveryState earlier, in XLOGShmemInit, based on
the ControlFile->state value. In my understanding, anything different than
DB_SHUTDOWNED or DB_SHUTDOWNED_IN_RECOVERY can be considered as a crash
recovery. Based on this XLOGShmemInit can init SharedRecoveryState to
IN_ARCHIVE_RECOVERY or IN_CRASH_RECOVERY.
With either values, RecoveryInProgress() keep returning the same result so any
current code relying on RecoveryInProgress() is compatible.
I'm not sure who would need this information before the WAL machinery is up,
but is it safe enough in your opinion for futur usage of GetRecoveryState()? Do
you think this information might be useful before the WAL machinery is set?
Current "user" (eg. restoreTwoPhaseData()) only need to know if we are in
recovery, whatever the reason.
The patch in attachment set SharedRecoveryState to either IN_ARCHIVE_RECOVERY
or IN_CRASH_RECOVERY from XLOGShmemInit based on the ControlFile state. It
feels strange though to set this so far away from ControlFile->state
= DB_IN_CRASH_RECOVERY...
> - if (!((XLogArchivingActive() && !inRecovery) ||
> - (XLogArchivingAlways() && inRecovery)))
> + if ( (inRecoveryState != IN_CRASH_RECOVERY) && (
> + (inRecoveryState == NOT_IN_RECOVERY
> && !XLogArchivingActive()) &&
> + (inRecoveryState == IN_ARCHIVE_RECOVERY
> && !XLogArchivingAlways()))) return true;
>
> The last condition seems to cause XLogArchiveCheckDone() to return
> true in archive recovery mode with archive_mode=on, then cause
> unarchived WAL files with .ready to be removed. Is my understanding right?
> If yes, that behavior doesn't seem to match with our consensus, i.e.,
> WAL files with .ready should not be removed in that case.
We 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.
So if in standby mode and archive_mode is not "always", the .ready files
should be removed.
In current patch, I split the conditions in the sake of clarity.
> +/* Recovery state */
> +typedef enum RecoveryState
> +{
> + NOT_IN_RECOVERY = 0,
> + IN_CRASH_RECOVERY,
> + IN_ARCHIVE_RECOVERY
> +} RecoveryState;
>
> Isn't it better to add more comments here? For example, what does
> "Recovery state" mean? Which state is used in standby mode? Why? ,etc.
Explanations added.
> Is it really ok not to have the value indicating standby mode?
Unless I'm wrong, this shared state only indicates why we are recovering WAL,
it does not need reflect the status of the instance in shared memory.
StandbyMode is already available for the startup process. Would it be useful
to share this mode outside of the startup process?
> These enum values names are confusing because the variables with
> similar names already exist. For example, IN_CRASH_RECOVERY vs.
> DB_IN_CRASH_RECOVERY. So IMO it seems better to rename them,
> .e.g., by adding the prefix.
Well, I lack of imagination. So I picked CRASH_RECOVERING and
ARCHIVE_RECOVERING.
> > "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.
>
> Ok, so firstly I try to review your patch!
Thank you for your review and help!
In attachment:
* fix proposal: 0001-v4-Fix-WAL-retention-during-production-crash-recovery.patch
* add test: 0002-v2-Add-test-on-non-archived-WAL-during-crash-recovery.patch
0002-v2 fix conflict with current master.
Regards,
Attachments:download all (2 files, .tar.gz)0001-v4-Fix-WAL-retention-during-production-crash-recovery.patch (text/x-patch, 7.0K) download
From 147f616565bc652950bcd928a870cd3aa3dfb805 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 | 45 +++++++++++++++++++-----
src/backend/access/transam/xlogarchive.c | 21 +++++++----
src/include/access/xlog.h | 23 ++++++++++++
3 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex abf954ba39..a7a5ce6ef4 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -220,8 +220,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 SharedRecoveryState is not ARCHIVE_RECOVERING.+ * True actually means "not known, need to check the shared state".
*/
static bool LocalRecoveryInProgress = true;
@@ -652,10 +652,10 @@ typedef struct XLogCtlData
TimeLineID PrevTimeLineID;
/*
- * SharedRecoveryInProgress indicates if we're still in crash or archive+ * SharedRecoveryState indicates if we're either in crash or archive
* recovery. Protected by info_lck.
*/
- bool SharedRecoveryInProgress;+ RecoveryState SharedRecoveryState;
/*
* SharedHotStandbyActive indicates if we allow hot standby queries to be
@@ -4407,6 +4407,11 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
updateMinRecoveryPoint = true;
UpdateControlFile();
++ SpinLockAcquire(&XLogCtl->info_lck);+ XLogCtl->SharedRecoveryState = ARCHIVE_RECOVERING;+ SpinLockRelease(&XLogCtl->info_lck);+
LWLockRelease(ControlFileLock);
CheckRecoveryConsistency();
@@ -5139,11 +5144,20 @@ XLOGShmemInit(void)
* in additional info.)
*/
XLogCtl->XLogCacheBlck = XLOGbuffers - 1;
- XLogCtl->SharedRecoveryInProgress = true;
XLogCtl->SharedHotStandbyActive = false;
XLogCtl->SharedPromoteIsTriggered = false;
XLogCtl->WalWriterSleeping = false;
+ switch (ControlFile->state)+ {+ case DB_SHUTDOWNED:+ case DB_SHUTDOWNED_IN_RECOVERY:+ XLogCtl->SharedRecoveryState = ARCHIVE_RECOVERING;+ break;+ default:+ XLogCtl->SharedRecoveryState = CRASH_RECOVERING;+ }+
SpinLockInit(&XLogCtl->Insert.insertpos_lck);
SpinLockInit(&XLogCtl->info_lck);
SpinLockInit(&XLogCtl->ulsn_lck);
@@ -7884,7 +7898,7 @@ StartupXLOG(void)
ControlFile->time = (pg_time_t) time(NULL);
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->SharedRecoveryInProgress = false;+ XLogCtl->SharedRecoveryState = NOT_IN_RECOVERY;
SpinLockRelease(&XLogCtl->info_lck);
UpdateControlFile();
@@ -8030,7 +8044,7 @@ RecoveryInProgress(void)
*/
volatile XLogCtlData *xlogctl = XLogCtl;
- LocalRecoveryInProgress = xlogctl->SharedRecoveryInProgress;+ LocalRecoveryInProgress = (xlogctl->SharedRecoveryState != NOT_IN_RECOVERY);
/*
* Initialize TimeLineID and RedoRecPtr when we discover that recovery
@@ -8042,8 +8056,8 @@ RecoveryInProgress(void)
{
/*
* If we just exited recovery, make sure we read TimeLineID and
- * RedoRecPtr after SharedRecoveryInProgress (for machines with- * weak memory ordering).+ * RedoRecPtr after SharedRecoveryState (for machines with weak+ * memory ordering).
*/
pg_memory_barrier();
InitXLOGAccess();
@@ -8059,6 +8073,19 @@ RecoveryInProgress(void)
}
}
+/*+ * Returns current recovery state from shared memory.+ *+ * See details about RecoveryState in xlog.h+ */+RecoveryState+GetRecoveryState(void)+{+ volatile XLogCtlData *xlogctl = XLogCtl;++ return xlogctl->SharedRecoveryState;+}+
/*
* 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..b207827651 100644--- a/src/backend/access/transam/xlogarchive.c+++ b/src/backend/access/transam/xlogarchive.c@@ -572,18 +572,25 @@ XLogArchiveCheckDone(const char *xlog)
{
char archiveStatusPath[MAXPGPATH];
struct stat stat_buf;
- bool inRecovery = RecoveryInProgress();+ RecoveryState recoveryState = GetRecoveryState();++ /* The file is always deletable if archive_mode is "off". */+ if ( ! XLogArchivingActive() )+ return true;
/*
- * 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".+ * On standbys, the file is deletable if archive_mode is not+ * "always".
*/
- if (!((XLogArchivingActive() && !inRecovery) ||- (XLogArchivingAlways() && inRecovery)))+ if ( recoveryState == ARCHIVE_RECOVERING && !XLogArchivingAlways() )
return true;
+ /*+ * We are either:+ * * a primary with archive_mode set to "on" or "always"+ * * a standby with archive_mode set to "always".+ */+
/* First check for .done --- this means archiver is done with it */
StatusFilePath(archiveStatusPath, xlog, ".done");
if (stat(archiveStatusPath, &stat_buf) == 0)
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.hindex 7412caa5f2..601ae34dd8 100644--- a/src/include/access/xlog.h+++ b/src/include/access/xlog.h@@ -165,6 +165,28 @@ typedef enum WalLevel
WAL_LEVEL_LOGICAL
} WalLevel;
+/*+ * Recovery state.+ *+ * There's two different reasons to recover WAL: when standby mode is requested+ * or after a crash to catchup with consistency.+ *+ * CRASH_RECOVERING is set when recovering after a crash.+ * ARCHIVE_RECOVERING is set when archive recovery or standby is requested.+ * NOT_IN_RECOVERY means the cluster is in production.+ *+ * Note that even if archive recovery or standby is requested, recovery state+ * starts first with CRASH_RECOVERING before switching to ARCHIVE_RECOVERING if:+ * - the control file shows evidence the cluster crashed based on its previous state+ * - there's no backup label file+ */+typedef enum RecoveryState+{+ NOT_IN_RECOVERY = 0, /* currently in production */+ CRASH_RECOVERING, /* recovering from a crash */+ ARCHIVE_RECOVERING /* recovering archives as requested */+} RecoveryState;+
extern PGDLLIMPORT int wal_level;
/* Is WAL archiving enabled (always or only while server is running normally)? */
@@ -279,6 +301,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-v2-Add-test-on-non-archived-WAL-during-crash-recovery.patch (text/x-patch, 1.5K) download
From 63feb240a6c7f9bdc305eb7b3aa0a25f3355ccbb 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 ca6e92b50d..ce2e899891 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 pg_current_xact_id();
+SELECT pg_switch_wal();
];
$tx->pump until $stdout =~ /[[:digit:]]+[\r\n]$/;
@@ -65,4 +72,9 @@ cmp_ok($node->safe_psql('postgres', 'SELECT pg_current_xact_id()'),
is($node->safe_psql('postgres', qq[SELECT pg_xact_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: <20200407171736.61906608@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