Date: Tue, 15 Feb 2022 22:57:32 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: USE_BARRIER_SMGRRELEASE on Linux?
Message-ID: <20220216065732.GA2932808@nathanxps13>
References: <20220215235845.GA2665318@nathanxps13>
 <CA+hUKGJxb5ug=sBL=S8WMsFg4mojccy8p1VWqXYEz4MSre_vGA@mail.gmail.com>
 <CA+hUKG+Ye6kBWkm9kjMhvAvO0gyrEy=q3rm05XSmZ2ah6auQNw@mail.gmail.com>
 <20220216045911.GA2791824@nathanxps13>
 <20220216050645.7u3nguccykpqaw7p@alap3.anarazel.de>
 <20220216061404.GA2813338@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="u3/rZRmxL6MmkK24"
Content-Disposition: inline
In-Reply-To: <20220216061404.GA2813338@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20220216065732.GA2932808%40nathanxps13>
Precedence: bulk


--u3/rZRmxL6MmkK24
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Feb 15, 2022 at 10:14:04PM -0800, Nathan Bossart wrote:
> It looks like register_unlink_segment() is called prior to the checkpoint,
> but the checkpointer is not calling RememberSyncRequest() until after
> SyncPreCheckpoint().  This means that the requests are registered with the
> next checkpoint cycle count, so they aren't processed until the next
> checkpoint.

Calling AbsorbSyncRequests() before advancing the checkpoint cycle counter
seems to fix the issue.  However, this requires moving SyncPreCheckpoint()
out of the critical section in CreateCheckPoint().  Patch attached.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--u3/rZRmxL6MmkK24
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-call-AbsorbSyncRequests-before-advancing-checkpoi.patch"

From 188fbe8ab12387c423081edfb85b26e43b342f4e Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 15 Feb 2022 22:53:04 -0800
Subject: [PATCH v1 1/1] call AbsorbSyncRequests() before advancing checkpoint
 cycle counter

---
 src/backend/access/transam/xlog.c | 14 +++++++-------
 src/backend/storage/sync/sync.c   |  6 ++++++
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 60d0f3d99e..c72d6faa88 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -9042,6 +9042,13 @@ CreateCheckPoint(int flags)
 	MemSet(&CheckpointStats, 0, sizeof(CheckpointStats));
 	CheckpointStats.ckpt_start_t = GetCurrentTimestamp();
 
+	/*
+	 * Let smgr prepare for checkpoint; this has to happen before we determine
+	 * the REDO pointer.  Note that smgr must not do anything that'd have to
+	 * be undone if we decide no checkpoint is needed.
+	 */
+	SyncPreCheckpoint();
+
 	/*
 	 * Use a critical section to force system panic if we have trouble.
 	 */
@@ -9055,13 +9062,6 @@ CreateCheckPoint(int flags)
 		LWLockRelease(ControlFileLock);
 	}
 
-	/*
-	 * Let smgr prepare for checkpoint; this has to happen before we determine
-	 * the REDO pointer.  Note that smgr must not do anything that'd have to
-	 * be undone if we decide no checkpoint is needed.
-	 */
-	SyncPreCheckpoint();
-
 	/* Begin filling in the checkpoint WAL record */
 	MemSet(&checkPoint, 0, sizeof(checkPoint));
 	checkPoint.time = (pg_time_t) time(NULL);
diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c
index 543f691f2d..8ffb3b8279 100644
--- a/src/backend/storage/sync/sync.c
+++ b/src/backend/storage/sync/sync.c
@@ -180,6 +180,12 @@ InitSync(void)
 void
 SyncPreCheckpoint(void)
 {
+	/*
+	 * Some operations assume that the next checkpoint will process recently
+	 * forwarded requests, so absorb them before we advance the cycle counter.
+	 */
+	AbsorbSyncRequests();
+
 	/*
 	 * Any unlink requests arriving after this point will be assigned the next
 	 * cycle counter, and won't be unlinked until next checkpoint.
-- 
2.25.1


--u3/rZRmxL6MmkK24--