Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish()

On 2022-11-25 16:54:19 +0530, Bharath Rupireddy wrote:
> On Fri, Nov 25, 2022 at 12:16 AM Andres Freund <andres(at)anarazel(dot)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.

> From 293e789f9c1a63748147acd613c556961f1dc5c4 Mon Sep 17 00:00:00 2001
> From: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)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

I'd probably split the change to an atomic from other changes either way.

Greetings,

Andres Freund