Re: WAL Insertion Lock Improvements

On Wed, May 10, 2023 at 5:34 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> + * NB: LWLockConflictsWithVar (which is called from
> + * LWLockWaitForVar) relies on the spinlock used above in this
> + * function and doesn't use a memory barrier.
>
> This patch adds the following comment in WaitXLogInsertionsToFinish()
> because lwlock.c on HEAD mentions that:
> /*
> * 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.
> */
>
> Should it be something where we'd better be noisy about at the top of
> LWLockWaitForVar()? We don't want to add a memory barrier at the
> beginning of LWLockConflictsWithVar(), still it strikes me that
> somebody that aims at using LWLockWaitForVar() may miss this point
> because LWLockWaitForVar() is the routine published in lwlock.h, not
> LWLockConflictsWithVar(). This does not need to be really
> complicated, say a note at the top of LWLockWaitForVar() among the
> lines of (?):
> "Be careful that LWLockConflictsWithVar() does not include a memory
> barrier, hence the caller of this function may want to rely on an
> explicit barrier or a spinlock to avoid memory ordering issues."

+1. Now, we have comments in 3 places to warn about the
LWLockConflictsWithVar not using memory barrier - one in
WaitXLogInsertionsToFinish, one in LWLockWaitForVar and another one
(existing) in LWLockConflictsWithVar specifying where exactly a memory
barrier is needed if the caller doesn't use a spinlock. Looks fine to
me.

> + * NB: pg_atomic_exchange_u64 is used here as opposed to just
> + * pg_atomic_write_u64 to update the variable. Since pg_atomic_exchange_u64
> + * is a full barrier, we're guaranteed that all loads and stores issued
> + * prior to setting the variable are completed before any loads or stores
> + * issued after setting the variable.
>
> This is the same explanation as LWLockUpdateVar(), except that we
> lose the details explaining why we are doing the update before
> releasing the lock.

I think what I have so far seems more verbose explaining what a
barrier does and all that. I honestly think we don't need to be that
verbose, thanks to README.barrier.

I simplified those 2 comments as the following:

* NB: pg_atomic_exchange_u64, having full barrier semantics will ensure
* the variable is updated before releasing the lock.

* NB: pg_atomic_exchange_u64, having full barrier semantics will ensure
* the variable is updated before waking up waiters.

Please find the attached v7 patch.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com