Re: Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations

Hi,

On 2026-03-10 19:41:54 +0100, Tomas Vondra wrote:
> On 2/5/26 16:38, Peter Geoghegan wrote:
> > On Wed, Jan 14, 2026 at 1:31 AM Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> >> Yeah, that was a quite big thinko. I have a attached a patch with the
> >> thinko fixed but I am still not happy with it. I think I will try to use
> >> atomics.h and see if that makes the code nicer to read.
> >>
> >> Also will after that see what I can do about pageinspect.
> >
> > I believe that pageinspect's heap_page_items function needs to use
> > get_page_from_raw -- see commit 14e9b18fed.

Maybe I'm slow today, but how's that related to the patch at hand?

Regardless of that, it seems a bit confusing to fold the pageinspect changes
into the main commit.

> The only real change is about asserts in BufferGetLSNAtomic( - the new
> "ifdef" branch only called PageGetLSN(), so it lost the checks that the
> buffer is valid and pinned. Which seems not desirable.
>
> In fact, looking at the existing code, shouldn't the asserts be before
> the "fastpath" exit (for cases without hint bits or local buffers)?
>
> The v4 changes both points. It adds the asserts to the new ifdef block,
> and moves it up in the existing one.
>
>
> regards
>
> --
> Tomas Vondra

> From 4d6479b37e60015cc4cf55579a8ec51337675996 Mon Sep 17 00:00:00 2001
> From: Andreas Karlsson <andreas(at)proxel(dot)se>
> Date: Fri, 21 Nov 2025 10:15:06 +0100
> Subject: [PATCH v4] Do not lock in BufferGetLSNAtomic() on archs with 8 byte
> atomic reads
>
> On platforms where we can read or write the whole LSN atomically, we do
> not need to lock the buffer header to prevent torn LSNs. We can do this
> only on platforms with PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY, and when the
> pd_lsn field is properly aligned.
>
> For historical reasons the PageXLogRecPtr was defined as a struct with
> two uint32 fields. This replaces it with a single uint64 value, to make
> the intent clearer.
>
> Idea by Andres Freund. Initial patch by Andreas Karlsson, improved by
> Peter Geoghegan. Minor tweaks by me.
>
> Author: Andreas Karlsson <andreas(at)proxel(dot)se>
> Author: Peter Geoghegan <pg(at)bowt(dot)ie>
> Reviewed-by: Andres Freund <andres(at)anarazel(dot)de>
> Reviewed-by: Tomas Vondra <tomas(at)vondra(dot)me>
> Discussion: https://postgr.es/m/b6610c3b-3f59-465a-bdbb-8e9259f0abc4@proxel.se

> diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
> index 5f3d083e938..efcf64169aa 100644
> --- a/src/backend/storage/buffer/bufmgr.c
> +++ b/src/backend/storage/buffer/bufmgr.c
> @@ -4628,33 +4628,42 @@ BufferIsPermanent(Buffer buffer)
>
> /*
> * BufferGetLSNAtomic
> - * Retrieves the LSN of the buffer atomically using a buffer header lock.
> - * This is necessary for some callers who may not have an exclusive lock
> - * on the buffer.
> + * Retrieves the LSN of the buffer atomically.
> + *
> + * On platforms without 8 byte atomic reads/write we need to take a
> + * header lock. This is necessary for some callers who may not have an
> + * exclusive lock on the buffer.
> */
> XLogRecPtr
> BufferGetLSNAtomic(Buffer buffer)
> {
> +#ifdef PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY
> + Assert(BufferIsValid(buffer));
> + Assert(BufferIsPinned(buffer));
> +
> + return PageGetLSN(BufferGetPage(buffer));
> +#else
> char *page = BufferGetPage(buffer);
> BufferDesc *bufHdr;
> XLogRecPtr lsn;
>
> + /* Make sure we've got a real buffer, and that we hold a pin on it. */
> + Assert(BufferIsValid(buffer));
> + Assert(BufferIsPinned(buffer));
> +

Seems a bit silly to have these asserts duplicated. I'd probably just put the
body of the #else into an {} and then have the asserts before the ifdef.

> diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
> index 92a6bb9b0c0..e994526ca52 100644
> --- a/src/include/storage/bufpage.h
> +++ b/src/include/storage/bufpage.h
> @@ -91,24 +91,27 @@ typedef uint16 LocationIndex;
>
>
> /*
> - * For historical reasons, the 64-bit LSN value is stored as two 32-bit
> - * values.
> + * For historical reasons, the storage of 64-bit LSN values depends on CPU
> + * endianness; PageXLogRecPtr used to be a struct consisting of two 32-bit
> + * values. When reading (and writing) the pd_lsn field from page headers, the
> + * caller must convert from (and convert to) the platform's native endianness.
> */
> -typedef struct
> -{
> - uint32 xlogid; /* high bits */
> - uint32 xrecoff; /* low bits */
> -} PageXLogRecPtr;
> +typedef uint64 PageXLogRecPtr;

I think this should explain why we are storing it as a 64bit value (i.e. that
we need to be able to read it without tearing).

I suspect this should continue to be a struct (just containing a 64bit uint),
otherwise it'll be way way too easy to omit the conversion, due to C's weak
typedefs.

> -static inline XLogRecPtr
> -PageXLogRecPtrGet(PageXLogRecPtr val)
> +/*
> + * Convert a pd_lsn taken from a page header into its native
> + * uint64/PageXLogRecPtr representation
> + */

Odd double space before pd_lsn.

> +static inline PageXLogRecPtr
> +PageXLogRecPtrGet(PageXLogRecPtr pd_lsn)
> {
> - return (uint64) val.xlogid << 32 | val.xrecoff;
> +#ifdef WORDS_BIGENDIAN
> + return pd_lsn;
> +#else
> + return (pd_lsn << 32) | (pd_lsn >> 32);
> +#endif
> }
>
> -#define PageXLogRecPtrSet(ptr, lsn) \
> - ((ptr).xlogid = (uint32) ((lsn) >> 32), (ptr).xrecoff = (uint32) (lsn))
> -
> /*
> * disk page organization
> *
> @@ -387,10 +390,11 @@ PageGetLSN(const PageData *page)
> {
> return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn);
> }

I think this may need to actually use a volatile to force the read to happen
as the 64bit value, otherwise the compiler would be entirely free to implement
it as two 32bit reads.

> static inline void
> PageSetLSN(Page page, XLogRecPtr lsn)
> {
> - PageXLogRecPtrSet(((PageHeader) page)->pd_lsn, lsn);
> + ((PageHeader) page)->pd_lsn = PageXLogRecPtrGet(lsn);
> }

Dito.

Seems also a bit misleading to document PageXLogRecPtrSet as "Convert a pd_lsn
taken from a page header into its native ..." when we use it for both
directions.

I think this probably also ought to assert that the page this is being set on
is properly aligned.

Greetings,

Andres Freund