Re: Atomic ops for unlogged LSN

On Wed, Nov 01, 2023 at 09:15:20PM +0000, John Morris wrote:
> This is a rebased version . Even though I labelled it “v3”, there should be no changes from “v2”.

Thanks.  I think this is almost ready, but I have to harp on the
pg_atomic_read_u64() business once more.  The relevant comment in atomics.h
has this note:

 * The read is guaranteed to return a value as it has been written by this or
 * another process at some point in the past. There's however no cache
 * coherency interaction guaranteeing the value hasn't since been written to
 * again.

However unlikely, this seems to suggest that CreateCheckPoint() could see
an old value with your patch.  Out of an abundance of caution, I'd
recommend changing this to pg_atomic_compare_exchange_u64() like
pg_atomic_read_u64_impl() does in generic.h.

@@ -4635,7 +4629,6 @@ XLOGShmemInit(void)
 
 	SpinLockInit(&XLogCtl->Insert.insertpos_lck);
 	SpinLockInit(&XLogCtl->info_lck);
-	SpinLockInit(&XLogCtl->ulsn_lck);
 }

Shouldn't we do the pg_atomic_init_u64() here?  We can still set the
initial value in StartupXLOG(), but it might be safer to initialize the
variable where we are initializing the other shared memory stuff.

Since this isn't a tremendously performance-sensitive area, IMHO we should
code defensively to eliminate any doubts about correctness and to make it
easier to reason about.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com