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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> > Author: Peter Geoghegan <[email protected]> > Reviewed-by: Andres Freund <[email protected]> > Reviewed-by: Tomas Vondra <[email protected]> > Discussion: https://postgr.es/m/[email protected] > 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
