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
