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
