On Fri, Dec 2, 2022 at 6:10 AM Andres Freund <and...@anarazel.de> wrote: > > On 2022-11-25 16:54:19 +0530, Bharath Rupireddy wrote: > > On Fri, Nov 25, 2022 at 12:16 AM Andres Freund <and...@anarazel.de> 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 <bharath.rupireddyforpostg...@gmail.com> > > 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%40mail.gmail.com -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v2-0001-Make-insertingAt-64-bit-atomic.patch
Description: Binary data
v2-0002-Add-fastpath-to-LWLockUpdateVar.patch
Description: Binary data
v2-0003-Make-lastImportantAt-64-bit-atomic.patch
Description: Binary data