From d07a88ec8862c35617a6ea6632d7b17869af5e67 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Sat, 22 Jul 2023 07:35:01 +0000 Subject: [PATCH v10] Improve commets in and around LWLockWaitForVar call sites This commit explains why it's safe to loop for LWLockWaitForVar without a memory barrier or a spinlock in WaitXLogInsertionsToFinish. In the loop we are only waiting for insertions that started before WaitXLogInsertionsToFinish was called. The lack of memory barriers in the loop means that we might see locks as "unused" that have since become used, which is fine, because they only can be for later insertions that we wouldn't want to wait on anyway. And, not taking a lock to acquire the current insertingAt value means that we might see older insertingAt value. Which is also be fine, because if we read a too old value, we will add ourselves to the queue, which contains atomic operations. --- src/backend/access/transam/xlog.c | 11 +++++++++++ src/backend/storage/lmgr/lwlock.c | 10 +++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 75814ac947..2fec31688c 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -1495,6 +1495,17 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto) * calling LWLockUpdateVar. But if it has to sleep, it will * advertise the insertion point with LWLockUpdateVar before * sleeping. + * + * In this loop we are only waiting for insertions that started + * before WaitXLogInsertionsToFinish was called. The lack of memory + * barriers in the loop means that we might see locks as "unused" + * that have since become used, which is fine, because they only + * can be for later insertions that we wouldn't want to wait on + * anyway. And, not taking a lock to acquire the current + * insertingAt value means that we might see older insertingAt + * value. Which is also be fine, because if we read a too old + * value, we will add ourselves to the queue, which contains atomic + * operations. */ if (LWLockWaitForVar(&WALInsertLocks[i].l.lock, &WALInsertLocks[i].l.insertingAt, diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index ffa865eb28..ec7b55ffa8 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -1556,9 +1556,9 @@ LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, /* * Test first to see if it the slot is free right now. * - * XXX: the caller uses a spinlock before this, so we don't need a memory - * barrier here as far as the current usage is concerned. But that might - * not be safe in general. + * XXX: the caller (WaitXLogInsertionsToFinish) uses an implied barrier via + * spinlock before this, so we don't need a memory barrier here as far as + * the current usage is concerned. But that might not be safe in general. */ mustwait = (pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE) != 0; @@ -1601,6 +1601,10 @@ LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, * * Note: this function ignores shared lock holders; if the lock is held * in shared mode, returns 'true'. + * + * Be aware that LWLockConflictsWithVar() does not include a memory barrier, + * hence the caller of this function may want to rely on an explicit barrier or + * an implied barrier via spinlock or LWLock to avoid memory ordering issues. */ bool LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, -- 2.34.1