On Fri, Dec 2, 2022 at 6:10 AM Andres Freund <andres@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 <andres@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.rupireddyforpostgres@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
