Re: LogwrtResult contended spinlock

Hi,

On 2021-02-02 20:19:19 -0300, Alvaro Herrera wrote:
> > > @@ -1169,6 +1169,8 @@ XLogInsertRecord(XLogRecData *rdata,
> > > {
> > > /* advance global request to include new block(s)
> > > */
> > > pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtRqst.Write, EndPos);
> > > + pg_memory_barrier();
> >
> > That's not really useful - the path that actually updates already
> > implies a barrier. It'd probably be better to add a barrier to a "never
> > executed cmpxchg" fastpath.
>
> Got it. Do you mean in pg_atomic_monotonic_advance_u64() itself?

Yes.

> I'm not sure which is the nicer semantics. (If it's got to be at the
> caller, then we'll need to return a boolean from there, which sounds
> worse.)

Nearly all other modifying atomic operations have full barrier
semantics, so I think it'd be better to have it inside the
pg_atomic_monotonic_advance_u64().

> +/*
> + * Monotonically advance the given variable using only atomic operations until
> + * it's at least the target value.
> + */
> +static inline void
> +pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
> +{
> + uint64 currval;
> +
> +#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
> + AssertPointerAlignment(ptr, 8);
> +#endif
> +
> + currval = pg_atomic_read_u64(ptr);
> + while (currval < target_)
> + {
> + if (pg_atomic_compare_exchange_u64(ptr, &currval, target_))
> + break;
> + }
> +}

So I think it'd be

static inline void
pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
{
uint64 currval;

#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
AssertPointerAlignment(ptr, 8);
#endif

currval = pg_atomic_read_u64(ptr);
if (currval >= target_)
{
pg_memory_barrier();
return;
}

while (currval < target_)
{
if (pg_atomic_compare_exchange_u64(ptr, &currval, target_))
break;
}
}

> @@ -1172,9 +1170,10 @@ XLogInsertRecord(XLogRecData *rdata,
> /* advance global request to include new block(s) */
> if (XLogCtl->LogwrtRqst.Write < EndPos)
> XLogCtl->LogwrtRqst.Write = EndPos;
> - /* update local result copy while I have the chance */
> - LogwrtResult = XLogCtl->LogwrtResult;
> SpinLockRelease(&XLogCtl->info_lck);
> + /* update local result copy */
> + LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
> + LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
> }

As mentioned before - it's not clear to me why this is a valid thing to
do without verifying all LogwrtResult.* usages. You can get updates
completely out of order / independently.

> @@ -2675,8 +2673,10 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
> * code in a couple of places.
> */
> {
> + pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Write, LogwrtResult.Write);
> + pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Flush, LogwrtResult.Flush);
> + pg_memory_barrier();
> SpinLockAcquire(&XLogCtl->info_lck);
> - XLogCtl->LogwrtResult = LogwrtResult;

I still don't see why we need "locked" atomic operations here, rather
than just a pg_atomic_write_u64(). They can only be modified
with WALWriteLock held. There's two reasons for using a spinlock in
this place:
1) it avoids torn reads of 64bit values -
pg_atomic_write_u64()/pg_atomic_read_u64() avoid that already.
2) it ensures that Write/Flush are updated in unison - but that's not
useful anymore, given that other places now read the variables
separately.

> @@ -3064,8 +3063,10 @@ XLogBackgroundFlush(void)
> WriteRqst.Write -= WriteRqst.Write % XLOG_BLCKSZ;
>
> /* if we have already flushed that far, consider async commit records */
> + LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
> if (WriteRqst.Write <= LogwrtResult.Flush)
> {
> + pg_memory_barrier();
> SpinLockAcquire(&XLogCtl->info_lck);
> WriteRqst.Write = XLogCtl->asyncXactLSN;
> SpinLockRelease(&XLogCtl->info_lck);

A SpinLockAcquire() is a full memory barrier on its own I think. This'd
probably better solved by just making asyncXactLSN atomic...

Greetings,

Andres Freund