Re: Atomic ops for unlogged LSN - Mailing list pgsql-hackers

From Bharath Rupireddy
Subject Re: Atomic ops for unlogged LSN
Date
Msg-id CALj2ACVBL2GoJmXbEFM5b1MVgkLA9JhKgzTTOJ2StnPwTePpAA@mail.gmail.com
Whole thread Raw
In response to Re: Atomic ops for unlogged LSN  (Nathan Bossart <nathandbossart@gmail.com>)
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Tomas Vondra
Date:
Subject: Re: Detoasting optionally to make Explain-Analyze less misleading
Next
From: Jeff Davis
Date:
Subject: Re: Fix search_path for all maintenance commands