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
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);
