public inbox for [email protected]help / color / mirror / Atom feed
Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish() 5+ messages / 3 participants [nested] [flat]
* Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish() @ 2022-12-02 00:40 Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2022-12-02 00:40 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On 2022-11-25 16:54:19 +0530, Bharath Rupireddy wrote: > On Fri, Nov 25, 2022 at 12:16 AM Andres Freund <[email protected]> wrote: > > I think we could improve this code more significantly by avoiding the call to > > LWLockWaitForVar() for all locks that aren't acquired or don't have a > > conflicting insertingAt, that'd require just a bit more work to handle systems > > without tear-free 64bit writes/reads. > > > > The easiest way would probably be to just make insertingAt a 64bit atomic, > > that transparently does the required work to make even non-atomic read/writes > > tear free. Then we could trivially avoid the spinlock in > > LWLockConflictsWithVar(), LWLockReleaseClearVar() and with just a bit more > > work add a fastpath to LWLockUpdateVar(). We don't need to acquire the wait > > list lock if there aren't any waiters. > > > > I'd bet that start to have visible effects in a workload with many small > > records. > > Thanks Andres! I quickly came up with the attached patch. I also ran > an insert test [1], below are the results. I also attached the results > graph. The cirrus-ci is happy with the patch - > https://github.com/BRupireddy/postgres/tree/wal_insertion_lock_improvements_v1_2. > Please let me know if the direction of the patch seems right. > clients HEAD PATCHED > 1 1354 1499 > 2 1451 1464 > 4 3069 3073 > 8 5712 5797 > 16 11331 11157 > 32 22020 22074 > 64 41742 42213 > 128 71300 76638 > 256 103652 118944 > 512 111250 161582 > 768 99544 161987 > 1024 96743 164161 > 2048 72711 156686 > 4096 54158 135713 Nice. > From 293e789f9c1a63748147acd613c556961f1dc5c4 Mon Sep 17 00:00:00 2001 > From: Bharath Rupireddy <[email protected]> > Date: Fri, 25 Nov 2022 10:53:56 +0000 > Subject: [PATCH v1] WAL Insertion Lock Improvements > > --- > src/backend/access/transam/xlog.c | 8 +++-- > src/backend/storage/lmgr/lwlock.c | 56 +++++++++++++++++-------------- > src/include/storage/lwlock.h | 7 ++-- > 3 files changed, 41 insertions(+), 30 deletions(-) > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > index a31fbbff78..b3f758abb3 100644 > --- a/src/backend/access/transam/xlog.c > +++ b/src/backend/access/transam/xlog.c > @@ -376,7 +376,7 @@ typedef struct XLogwrtResult > typedef struct > { > LWLock lock; > - XLogRecPtr insertingAt; > + pg_atomic_uint64 insertingAt; > XLogRecPtr lastImportantAt; > } WALInsertLock; > > @@ -1482,6 +1482,10 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto) > { > XLogRecPtr insertingat = InvalidXLogRecPtr; > > + /* Quickly check and continue if no one holds the lock. */ > + if (!IsLWLockHeld(&WALInsertLocks[i].l.lock)) > + continue; I'm not sure this is quite right - don't we need a memory barrier. But I don't see a reason to not just leave this code as-is. I think this should be optimized entirely in lwlock.c I'd probably split the change to an atomic from other changes either way. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) @ 2022-12-02 11:02 Bharath Rupireddy <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Bharath Rupireddy @ 2022-12-02 11:02 UTC (permalink / raw) To: Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Dec 2, 2022 at 6:10 AM Andres Freund <[email protected]> wrote: > > On 2022-11-25 16:54:19 +0530, Bharath Rupireddy wrote: > > On Fri, Nov 25, 2022 at 12:16 AM Andres Freund <[email protected]> wrote: > > > I think we could improve this code more significantly by avoiding the call to > > > LWLockWaitForVar() for all locks that aren't acquired or don't have a > > > conflicting insertingAt, that'd require just a bit more work to handle systems > > > without tear-free 64bit writes/reads. > > > > > > The easiest way would probably be to just make insertingAt a 64bit atomic, > > > that transparently does the required work to make even non-atomic read/writes > > > tear free. Then we could trivially avoid the spinlock in > > > LWLockConflictsWithVar(), LWLockReleaseClearVar() and with just a bit more > > > work add a fastpath to LWLockUpdateVar(). We don't need to acquire the wait > > > list lock if there aren't any waiters. > > > > > > I'd bet that start to have visible effects in a workload with many small > > > records. > > > > Thanks Andres! I quickly came up with the attached patch. I also ran > > an insert test [1], below are the results. I also attached the results > > graph. The cirrus-ci is happy with the patch - > > https://github.com/BRupireddy/postgres/tree/wal_insertion_lock_improvements_v1_2. > > Please let me know if the direction of the patch seems right. > > clients HEAD PATCHED > > 1 1354 1499 > > 2 1451 1464 > > 4 3069 3073 > > 8 5712 5797 > > 16 11331 11157 > > 32 22020 22074 > > 64 41742 42213 > > 128 71300 76638 > > 256 103652 118944 > > 512 111250 161582 > > 768 99544 161987 > > 1024 96743 164161 > > 2048 72711 156686 > > 4096 54158 135713 > > Nice. Thanks for taking a look at it. > > From 293e789f9c1a63748147acd613c556961f1dc5c4 Mon Sep 17 00:00:00 2001 > > From: Bharath Rupireddy <[email protected]> > > Date: Fri, 25 Nov 2022 10:53:56 +0000 > > Subject: [PATCH v1] WAL Insertion Lock Improvements > > > > --- > > src/backend/access/transam/xlog.c | 8 +++-- > > src/backend/storage/lmgr/lwlock.c | 56 +++++++++++++++++-------------- > > src/include/storage/lwlock.h | 7 ++-- > > 3 files changed, 41 insertions(+), 30 deletions(-) > > > > diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c > > index a31fbbff78..b3f758abb3 100644 > > --- a/src/backend/access/transam/xlog.c > > +++ b/src/backend/access/transam/xlog.c > > @@ -376,7 +376,7 @@ typedef struct XLogwrtResult > > typedef struct > > { > > LWLock lock; > > - XLogRecPtr insertingAt; > > + pg_atomic_uint64 insertingAt; > > XLogRecPtr lastImportantAt; > > } WALInsertLock; > > > > @@ -1482,6 +1482,10 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto) > > { > > XLogRecPtr insertingat = InvalidXLogRecPtr; > > > > + /* Quickly check and continue if no one holds the lock. */ > > + if (!IsLWLockHeld(&WALInsertLocks[i].l.lock)) > > + continue; > > I'm not sure this is quite right - don't we need a memory barrier. But I don't > see a reason to not just leave this code as-is. I think this should be > optimized entirely in lwlock.c Actually, we don't need that at all as LWLockWaitForVar() will return immediately if the lock is free. So, I removed it. > I'd probably split the change to an atomic from other changes either way. Done. I've added commit messages to each of the patches. I've also brought the patch from [1] here as 0003. Thoughts? [1] https://www.postgresql.org/message-id/CALj2ACXtQdrGXtb%3DrbUOXddm1wU1vD9z6q_39FQyX0166dq%3D%3DA%40ma... -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com Attachments: [application/octet-stream] v2-0001-Make-insertingAt-64-bit-atomic.patch (6.2K, ../../CALj2ACXYpYDboe4i+yXDYoNQj9-G0584hP5x_b=W6hj693qcaA@mail.gmail.com/2-v2-0001-Make-insertingAt-64-bit-atomic.patch) download | inline diff: From bbc589de76cd37ffa6f7969f07b4f750a7911846 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 2 Dec 2022 08:46:23 +0000 Subject: [PATCH v2] Make insertingAt 64-bit atomic WAL insertion lwlock's insertingAt value is currently read/cleared with the help of lwlock's wait list lock to avoid torn-free reads as it is of type XLogRecPtr. This wait list lock can become a point of contention on a highly concurrent write workloads. Therefore, make insertingAt a 64-bit atomic which inherently provides torn-free reads and writes. --- src/backend/access/transam/xlog.c | 4 +-- src/backend/storage/lmgr/lwlock.c | 44 +++++++++++-------------------- src/include/storage/lwlock.h | 6 ++--- 3 files changed, 20 insertions(+), 34 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index c153c32a77..26e841ef6c 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -376,7 +376,7 @@ typedef struct XLogwrtResult typedef struct { LWLock lock; - XLogRecPtr insertingAt; + pg_atomic_uint64 insertingAt; XLogRecPtr lastImportantAt; } WALInsertLock; @@ -4602,7 +4602,7 @@ XLOGShmemInit(void) for (i = 0; i < NUM_XLOGINSERT_LOCKS; i++) { LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT); - WALInsertLocks[i].l.insertingAt = InvalidXLogRecPtr; + pg_atomic_init_u64(&WALInsertLocks[i].l.insertingAt, InvalidXLogRecPtr); WALInsertLocks[i].l.lastImportantAt = InvalidXLogRecPtr; } diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index a5ad36ca78..f7556dcb7d 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -1546,9 +1546,8 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode) * *result is set to true if the lock was free, and false otherwise. */ static bool -LWLockConflictsWithVar(LWLock *lock, - uint64 *valptr, uint64 oldval, uint64 *newval, - bool *result) +LWLockConflictsWithVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, + uint64 *newval, bool *result) { bool mustwait; uint64 value; @@ -1570,14 +1569,7 @@ LWLockConflictsWithVar(LWLock *lock, *result = false; - /* - * Read value using the lwlock's wait list lock, as we can't generally - * rely on atomic 64 bit reads/stores. TODO: On platforms with a way to - * do atomic 64 bit reads/writes the spinlock should be optimized away. - */ - LWLockWaitListLock(lock); - value = *valptr; - LWLockWaitListUnlock(lock); + value = pg_atomic_read_u64(valptr); if (value != oldval) { @@ -1606,7 +1598,8 @@ LWLockConflictsWithVar(LWLock *lock, * in shared mode, returns 'true'. */ bool -LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval) +LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, + uint64 *newval) { PGPROC *proc = MyProc; int extraWaits = 0; @@ -1734,29 +1727,29 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval) * LWLockUpdateVar - Update a variable and wake up waiters atomically * * Sets *valptr to 'val', and wakes up all processes waiting for us with - * LWLockWaitForVar(). Setting the value and waking up the processes happen - * atomically so that any process calling LWLockWaitForVar() on the same lock - * is guaranteed to see the new value, and act accordingly. + * LWLockWaitForVar(). It first sets the value atomically and then wakes up + * the waiting processes so that any process calling LWLockWaitForVar() on the + * same lock is guaranteed to see the new value, and act accordingly. * * The caller must be holding the lock in exclusive mode. */ void -LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val) +LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val) { proclist_head wakeup; proclist_mutable_iter iter; PRINT_LWDEBUG("LWLockUpdateVar", lock, LW_EXCLUSIVE); + /* Update the lock's value atomically first. */ + pg_atomic_write_u64(valptr, val); + proclist_init(&wakeup); LWLockWaitListLock(lock); Assert(pg_atomic_read_u32(&lock->state) & LW_VAL_EXCLUSIVE); - /* Update the lock's value */ - *valptr = val; - /* * See if there are any LW_WAIT_UNTIL_FREE waiters that need to be woken * up. They are always in the front of the queue. @@ -1872,17 +1865,10 @@ LWLockRelease(LWLock *lock) * LWLockReleaseClearVar - release a previously acquired lock, reset variable */ void -LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val) +LWLockReleaseClearVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val) { - LWLockWaitListLock(lock); - - /* - * Set the variable's value before releasing the lock, that prevents race - * a race condition wherein a new locker acquires the lock, but hasn't yet - * set the variables value. - */ - *valptr = val; - LWLockWaitListUnlock(lock); + /* Update the lock's value atomically */ + pg_atomic_write_u64(valptr, val); LWLockRelease(lock); } diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index a494cb598f..4227e59298 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -125,14 +125,14 @@ extern bool LWLockAcquire(LWLock *lock, LWLockMode mode); extern bool LWLockConditionalAcquire(LWLock *lock, LWLockMode mode); extern bool LWLockAcquireOrWait(LWLock *lock, LWLockMode mode); extern void LWLockRelease(LWLock *lock); -extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val); +extern void LWLockReleaseClearVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val); extern void LWLockReleaseAll(void); extern bool LWLockHeldByMe(LWLock *lock); extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride); extern bool LWLockHeldByMeInMode(LWLock *lock, LWLockMode mode); -extern bool LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval); -extern void LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val); +extern bool LWLockWaitForVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 oldval, uint64 *newval); +extern void LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val); extern Size LWLockShmemSize(void); extern void CreateLWLocks(void); -- 2.34.1 [application/octet-stream] v2-0002-Add-fastpath-to-LWLockUpdateVar.patch (1.1K, ../../CALj2ACXYpYDboe4i+yXDYoNQj9-G0584hP5x_b=W6hj693qcaA@mail.gmail.com/3-v2-0002-Add-fastpath-to-LWLockUpdateVar.patch) download | inline diff: From ab047556c0bc93bc6f5d28cef1975ae9ccdf46f8 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 2 Dec 2022 10:30:37 +0000 Subject: [PATCH v2] Add fastpath to LWLockUpdateVar() Add fastpath to LWLockUpdateVar() when there are no waiters. This avoids unnecessary lwlock's wait list lock acquisition and release. --- src/backend/storage/lmgr/lwlock.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index f7556dcb7d..9d90cf9adf 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -1744,6 +1744,13 @@ LWLockUpdateVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val) /* Update the lock's value atomically first. */ pg_atomic_write_u64(valptr, val); + /* + * Quick exit when there are no waiters. This avoids unnecessary lwlock's + * wait list lock acquisition and release. + */ + if ((pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS) == 0) + return; + proclist_init(&wakeup); LWLockWaitListLock(lock); -- 2.34.1 [application/octet-stream] v2-0003-Make-lastImportantAt-64-bit-atomic.patch (2.3K, ../../CALj2ACXYpYDboe4i+yXDYoNQj9-G0584hP5x_b=W6hj693qcaA@mail.gmail.com/4-v2-0003-Make-lastImportantAt-64-bit-atomic.patch) download | inline diff: From 5b1380553e69f1f85343441697d7c6af0b6a0727 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Fri, 2 Dec 2022 09:24:16 +0000 Subject: [PATCH v2] Make lastImportantAt 64-bit atomic WAL insertion lwlock's lastImportantAt value is currently read by GetLastImportantRecPtr() with the help of insertion lock acquisition and release to avoid torn-free reads. Make it a 64-bit atomic, which inherently provides torn-free reads and writes, to reduce unnecessary WAL insertion lock acquisitions and releases. --- src/backend/access/transam/xlog.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index ec289e0f70..f40e82ba5f 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -377,7 +377,7 @@ typedef struct { LWLock lock; pg_atomic_uint64 insertingAt; - XLogRecPtr lastImportantAt; + pg_atomic_uint64 lastImportantAt; } WALInsertLock; /* @@ -873,7 +873,7 @@ XLogInsertRecord(XLogRecData *rdata, { int lockno = holdingAllLocks ? 0 : MyLockNo; - WALInsertLocks[lockno].l.lastImportantAt = StartPos; + pg_atomic_write_u64(&WALInsertLocks[lockno].l.lastImportantAt, StartPos); } } else @@ -4603,7 +4603,7 @@ XLOGShmemInit(void) { LWLockInitialize(&WALInsertLocks[i].l.lock, LWTRANCHE_WAL_INSERT); pg_atomic_init_u64(&WALInsertLocks[i].l.insertingAt, InvalidXLogRecPtr); - WALInsertLocks[i].l.lastImportantAt = InvalidXLogRecPtr; + pg_atomic_write_u64(&WALInsertLocks[i].l.lastImportantAt, InvalidXLogRecPtr); } /* @@ -6124,13 +6124,10 @@ GetLastImportantRecPtr(void) XLogRecPtr last_important; /* - * Need to take a lock to prevent torn reads of the LSN, which are - * possible on some of the supported platforms. WAL insert locks only - * support exclusive mode, so we have to use that. + * We atomically read lastImportantAt which prevents torn reads. Hence + * no need to take WAL insert lock here. */ - LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE); - last_important = WALInsertLocks[i].l.lastImportantAt; - LWLockRelease(&WALInsertLocks[i].l.lock); + last_important = pg_atomic_read_u64(&WALInsertLocks[i].l.lastImportantAt); if (res < last_important) res = last_important; -- 2.34.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) @ 2022-12-03 00:31 Nathan Bossart <[email protected]> parent: Bharath Rupireddy <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Nathan Bossart @ 2022-12-03 00:31 UTC (permalink / raw) To: Bharath Rupireddy <[email protected]>; +Cc: Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]> On Fri, Dec 02, 2022 at 04:32:38PM +0530, Bharath Rupireddy wrote: > On Fri, Dec 2, 2022 at 6:10 AM Andres Freund <[email protected]> wrote: >> I'm not sure this is quite right - don't we need a memory barrier. But I don't >> see a reason to not just leave this code as-is. I think this should be >> optimized entirely in lwlock.c > > Actually, we don't need that at all as LWLockWaitForVar() will return > immediately if the lock is free. So, I removed it. I briefly looked at the latest patch set, and I'm curious how this change avoids introducing memory ordering bugs. Perhaps I am missing something obvious. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) @ 2022-12-05 18:30 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2022-12-05 18:30 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; PostgreSQL Hackers <[email protected]> Hi, FWIW, I don't see an advantage in 0003. If it allows us to make something else simpler / faster, cool, but on its own it doesn't seem worthwhile. On 2022-12-02 16:31:58 -0800, Nathan Bossart wrote: > On Fri, Dec 02, 2022 at 04:32:38PM +0530, Bharath Rupireddy wrote: > > On Fri, Dec 2, 2022 at 6:10 AM Andres Freund <[email protected]> wrote: > >> I'm not sure this is quite right - don't we need a memory barrier. But I don't > >> see a reason to not just leave this code as-is. I think this should be > >> optimized entirely in lwlock.c > > > > Actually, we don't need that at all as LWLockWaitForVar() will return > > immediately if the lock is free. So, I removed it. > > I briefly looked at the latest patch set, and I'm curious how this change > avoids introducing memory ordering bugs. Perhaps I am missing something > obvious. I'm a bit confused too - the comment above talks about LWLockWaitForVar(), but the patches seem to optimize LWLockUpdateVar(). I think it'd be safe to optimize LWLockConflictsWithVar(), due to some pre-existing, quite crufty, code. LWLockConflictsWithVar() says: * 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. which happens to be true in the single, indirect, caller: /* Read the current insert position */ SpinLockAcquire(&Insert->insertpos_lck); bytepos = Insert->CurrBytePos; SpinLockRelease(&Insert->insertpos_lck); reservedUpto = XLogBytePosToEndRecPtr(bytepos); I think at the very least we ought to have a comment in WaitXLogInsertionsToFinish() highlighting this. It's not at all clear to me that the proposed fast-path for LWLockUpdateVar() is safe. I think at the very least we could end up missing waiters that we should have woken up. I think it ought to be safe to do something like pg_atomic_exchange_u64().. if (!(pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS)) return; because the pg_atomic_exchange_u64() will provide the necessary memory barrier. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) @ 2022-12-08 06:59 Bharath Rupireddy <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bharath Rupireddy @ 2022-12-08 06:59 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Nathan Bossart <[email protected]>; PostgreSQL Hackers <[email protected]> On Tue, Dec 6, 2022 at 12:00 AM Andres Freund <[email protected]> wrote: > > FWIW, I don't see an advantage in 0003. If it allows us to make something else > simpler / faster, cool, but on its own it doesn't seem worthwhile. Thanks. I will discard it. > I think it'd be safe to optimize LWLockConflictsWithVar(), due to some > pre-existing, quite crufty, code. LWLockConflictsWithVar() says: > > * 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. > > which happens to be true in the single, indirect, caller: > > /* Read the current insert position */ > SpinLockAcquire(&Insert->insertpos_lck); > bytepos = Insert->CurrBytePos; > SpinLockRelease(&Insert->insertpos_lck); > reservedUpto = XLogBytePosToEndRecPtr(bytepos); > > I think at the very least we ought to have a comment in > WaitXLogInsertionsToFinish() highlighting this. So, using a spinlock ensures no memory ordering occurs while reading lock->state in LWLockConflictsWithVar()? How does spinlock that gets acquired and released in the caller WaitXLogInsertionsToFinish() itself and the memory barrier in the called function LWLockConflictsWithVar() relate here? Can you please help me understand this a bit? > It's not at all clear to me that the proposed fast-path for LWLockUpdateVar() > is safe. I think at the very least we could end up missing waiters that we > should have woken up. > > I think it ought to be safe to do something like > > pg_atomic_exchange_u64().. > if (!(pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS)) > return; pg_atomic_exchange_u64(&lock->state, exchange_with_what_?. Exchange will change the value no? > because the pg_atomic_exchange_u64() will provide the necessary memory > barrier. I'm reading some comments [1], are these also true for 64-bit atomic CAS? Does it mean that an atomic CAS operation inherently provides a memory barrier? Can you please point me if it's described better somewhere else? [1] * Full barrier semantics. */ static inline uint32 pg_atomic_exchange_u32(volatile pg_atomic_uint32 *ptr, /* * Get and clear the flags that are set for this backend. Note that * pg_atomic_exchange_u32 is a full barrier, so we're guaranteed that the * read of the barrier generation above happens before we atomically * extract the flags, and that any subsequent state changes happen * afterward. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-12-08 06:59 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-12-02 00:40 Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish() Andres Freund <[email protected]> 2022-12-02 11:02 ` WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) Bharath Rupireddy <[email protected]> 2022-12-03 00:31 ` Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) Nathan Bossart <[email protected]> 2022-12-05 18:30 ` Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) Andres Freund <[email protected]> 2022-12-08 06:59 ` Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()) Bharath Rupireddy <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox