Date: Mon, 6 Nov 2023 14:33:50 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: John Morris <john.morris@crunchydata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Stephen Frost <sfrost@snowman.net>,
	Michael Paquier <michael@paquier.xyz>,
	Robert Haas <robertmhaas@gmail.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Atomic ops for unlogged LSN
Message-ID: <20231106203350.GA598180@nathanxps13>
References: <MN2PR13MB2688FD8B757316CB5C54C8A2A0DDA@MN2PR13MB2688.namprd13.prod.outlook.com>
 <20231026203433.GA1088329@nathanxps13>
 <BYAPR13MB26779988F4C73DD395401C59A0DCA@BYAPR13MB2677.namprd13.prod.outlook.com>
 <BYAPR13MB267762560386F4FB5BFDA235A0A7A@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231102034006.GA85609@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="5mCyUwZo2JvN/JJP"
Content-Disposition: inline
In-Reply-To: <20231102034006.GA85609@nathanxps13>
Archived-At: <https://www.postgresql.org/message-id/20231106203350.GA598180%40nathanxps13>
Precedence: bulk


--5mCyUwZo2JvN/JJP
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Nov 01, 2023 at 10:40:06PM -0500, Nathan Bossart wrote:
> Since this isn't a tremendously performance-sensitive area, IMHO we should
> code defensively to eliminate any doubts about correctness and to make it
> easier to reason about.

Concretely, like this.

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

--5mCyUwZo2JvN/JJP
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="unlogged_lsn_v4.patch"

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b541be8eec..1068f96c2d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -469,9 +469,8 @@ typedef struct XLogCtlData
 
 	XLogSegNo	lastRemovedSegNo;	/* latest removed/recycled XLOG segment */
 
-	/* Fake LSN counter, for unlogged relations. Protected by ulsn_lck. */
-	XLogRecPtr	unloggedLSN;
-	slock_t		ulsn_lck;
+	/* Fake LSN counter, for unlogged relations. */
+	pg_atomic_uint64 unloggedLSN;
 
 	/* Time and LSN of last xlog segment switch. Protected by WALWriteLock. */
 	pg_time_t	lastSegSwitchTime;
@@ -4294,14 +4293,7 @@ DataChecksumsEnabled(void)
 XLogRecPtr
 GetFakeLSNForUnloggedRel(void)
 {
-	XLogRecPtr	nextUnloggedLSN;
-
-	/* increment the unloggedLSN counter, need SpinLock */
-	SpinLockAcquire(&XLogCtl->ulsn_lck);
-	nextUnloggedLSN = XLogCtl->unloggedLSN++;
-	SpinLockRelease(&XLogCtl->ulsn_lck);
-
-	return nextUnloggedLSN;
+	return pg_atomic_fetch_add_u64(&XLogCtl->unloggedLSN, 1);
 }
 
 /*
@@ -4714,7 +4706,7 @@ XLOGShmemInit(void)
 
 	SpinLockInit(&XLogCtl->Insert.insertpos_lck);
 	SpinLockInit(&XLogCtl->info_lck);
-	SpinLockInit(&XLogCtl->ulsn_lck);
+	pg_atomic_init_u64(&XLogCtl->unloggedLSN, 0);
 }
 
 /*
@@ -5319,9 +5311,9 @@ StartupXLOG(void)
 	 * the unlogged LSN counter can be reset too.
 	 */
 	if (ControlFile->state == DB_SHUTDOWNED)
-		XLogCtl->unloggedLSN = ControlFile->unloggedLSN;
+		pg_atomic_write_u64(&XLogCtl->unloggedLSN, ControlFile->unloggedLSN);
 	else
-		XLogCtl->unloggedLSN = FirstNormalUnloggedLSN;
+		pg_atomic_write_u64(&XLogCtl->unloggedLSN, FirstNormalUnloggedLSN);
 
 	/*
 	 * Copy any missing timeline history files between 'now' and the recovery
@@ -6902,10 +6894,12 @@ CreateCheckPoint(int flags)
 	 * Persist unloggedLSN value. It's reset on crash recovery, so this goes
 	 * unused on non-shutdown checkpoints, but seems useful to store it always
 	 * for debugging purposes.
+	 *
+	 * pg_atomic_read_u64() isn't guaranteed to return the most up-to-date
+	 * value, so this is implemented via a compare/exchange with 0 to ensure
+	 * the necessary cache coherency interactions.
 	 */
-	SpinLockAcquire(&XLogCtl->ulsn_lck);
-	ControlFile->unloggedLSN = XLogCtl->unloggedLSN;
-	SpinLockRelease(&XLogCtl->ulsn_lck);
+	pg_atomic_compare_exchange_u64(&XLogCtl->unloggedLSN, &ControlFile->unloggedLSN, 0);
 
 	UpdateControlFile();
 	LWLockRelease(ControlFileLock);

--5mCyUwZo2JvN/JJP--