Re: WAL Insertion Lock Improvements

On Wed, May 10, 2023 at 5:34 PM Michael Paquier <michael@paquier.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