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

Hi,

On 2026-03-11 01:43:02 +0100, Tomas Vondra wrote:
> >>> 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.
> >>>
> >>
> >> That's because of alignment requirements, per this comment:
> >>
> >> * On machines with MAXALIGN = 8, the payload of a bytea is not
> >> * maxaligned, since it will start 4 bytes into a palloc'd value.
> >> * PageHeaderData requires 8 byte alignment, so always use this
> >> * function when accessing page header fields from a raw page bytea.
> >
> > I guess we are now reading 8 bytes as one.
> >
>
> Not sure I understand. Are you saying it's OK or not?

I mean that this adds an 8 byte read, which alignment picky hardware could
complain about, in theory. So I guess I kinda see the point of the change
now.

> >> I'm also a bit ... unsure about "volatile" in general. Is that actually
> >> something the specification says is needed here, or is it just an
> >> attempt to "disable optimizations" (like the split into two stores)?
> >
> > It's definitely suboptimal. We should use something C11's
> > atomic_load()/atomic_store(). But we can't rely on that yet (it's not enabled
> > without extra flags in at least msvc).
> >
> > The volatile does prevent the compiler from just doing a read/write twice or
> > never, just because it'd be nicer for code generation. But it doesn't
> > actually guarantee that the right instructions for reading writing are
> > generated :(
> >
>
> That sounds a bit scary, TBH.

Indeed. It's how atomic reads / writes have worked for quite a while though,
so I think we'll just have to live with it until we can rely on C11 atomics on
msvc.

> From 794fb844266dcd8d97217da09b61c5c9cafc01d7 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 v5] 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.

I'd add a note explaining why heapfuncs is updated.

> /*
> * disk page organization
> @@ -385,12 +411,13 @@ PageGetMaxOffsetNumber(const PageData *page)
> static inline XLogRecPtr
> PageGetLSN(const PageData *page)
> {
> - return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn);
> + return PageXLogRecPtrGet(&((const volatile PageHeaderData *) page)->pd_lsn);
> }

I don't think this volatile is needed, the one in PageXLogRecPtrGet's
declaration should do the work.

Greetings,

Andres Freund