On Thu, Nov 2, 2023 at 9:10 AM Nathan Bossart <nathandboss...@gmail.com> wrote: > > 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.
+1. pg_atomic_read_u64 provides no barrier semantics whereas pg_atomic_compare_exchange_u64 does. Without the barrier, it might happen that the value is read while the other backend is changing it. I think something like below providing full barrier semantics looks fine to me: XLogRecPtr ulsn; pg_atomic_compare_exchange_u64(&XLogCtl->unloggedLSN, &ulsn, 0); ControlFile->unloggedLSN = ulsn; > @@ -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. I think no one accesses the unloggedLSN in between CreateSharedMemoryAndSemaphores -> XLOGShmemInit and StartupXLOG. However, I see nothing wrong in doing pg_atomic_init_u64(&XLogCtl->unloggedLSN, InvalidXLogRecPtr); in XLOGShmemInit. > 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. Right. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
