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
