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