Date: Wed, 1 Apr 2020 18:17:35 +0200
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
Message-ID: <20200401181735.11100908@firost>
In-Reply-To: <d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
References: <20200331172229.40ee00dc@firost>
	<d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/h7vEWdLbZK.sp3iu_oxK3Oq"
Precedence: bulk

--MP_/h7vEWdLbZK.sp3iu_oxK3Oq
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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,

--MP_/h7vEWdLbZK.sp3iu_oxK3Oq
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-v2-Fix-WAL-retention-during-production-crash-recovery.patch

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.c
index 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.c
index 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.h
index 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


--MP_/h7vEWdLbZK.sp3iu_oxK3Oq
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0002-v1-Add-test-on-non-archived-WAL-during-crash-recovery.patch

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.pl
index 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


--MP_/h7vEWdLbZK.sp3iu_oxK3Oq--