On Thu, Nov 2, 2023 at 9:10 AM Nathan Bossart <nathandbossart@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
