On 1/2/26 4:27 PM, Andres Freund wrote:
A volatile on a non-pointer won't do you much good, I'm afraid. You need to make sure that the underlying value is read as a single 8 byte read, I don't see how this guarantees that, unfortunately.
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. Andreas
From 80ff6ef3744034e8c34aa1cd1c2dfb9cca013905 Mon Sep 17 00:00:00 2001 From: Andreas Karlsson <[email protected]> Date: Fri, 21 Nov 2025 10:15:06 +0100 Subject: [PATCH v2] Do not lock in BufferGetLSNAtomic() on archs with 8 byte atomic reads On platforms where we can read or write the whole LSN to a buffer page we do not need to lock the buffer header to be sure we do not get a torn LSN. To ahcieve this we need to make sure we always read and write the while LSN and do not write it field by field. While working on this I converted PageXLogRecPtr from a struct with two uint32 fields into an uint64 to make the intent more clear. Idea by Andres Freund. --- src/backend/access/common/bufmask.c | 2 +- src/backend/storage/buffer/bufmgr.c | 12 ++++++-- src/include/access/gist.h | 4 +-- src/include/storage/bufpage.h | 46 +++++++++++++++++++++-------- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/backend/access/common/bufmask.c b/src/backend/access/common/bufmask.c index 1a9e7bea5d2..8a67bfa1aff 100644 --- a/src/backend/access/common/bufmask.c +++ b/src/backend/access/common/bufmask.c @@ -32,7 +32,7 @@ mask_page_lsn_and_checksum(Page page) { PageHeader phdr = (PageHeader) page; - PageXLogRecPtrSet(phdr->pd_lsn, (uint64) MASK_MARKER); + PageXLogRecPtrSet(&phdr->pd_lsn, (uint64) MASK_MARKER); phdr->pd_checksum = MASK_MARKER; } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index ea5cb30fed0..58de896fa7c 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -4586,13 +4586,18 @@ 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 + return PageGetLSN(BufferGetPage(buffer)); +#else char *page = BufferGetPage(buffer); BufferDesc *bufHdr; XLogRecPtr lsn; @@ -4613,6 +4618,7 @@ BufferGetLSNAtomic(Buffer buffer) UnlockBufHdr(bufHdr); return lsn; +#endif } /* --------------------------------------------------------------------- diff --git a/src/include/access/gist.h b/src/include/access/gist.h index 8fa30126f8f..9b385b13a88 100644 --- a/src/include/access/gist.h +++ b/src/include/access/gist.h @@ -186,8 +186,8 @@ typedef struct GISTENTRY #define GistMarkFollowRight(page) ( GistPageGetOpaque(page)->flags |= F_FOLLOW_RIGHT) #define GistClearFollowRight(page) ( GistPageGetOpaque(page)->flags &= ~F_FOLLOW_RIGHT) -#define GistPageGetNSN(page) ( PageXLogRecPtrGet(GistPageGetOpaque(page)->nsn)) -#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)->nsn, val)) +#define GistPageGetNSN(page) ( PageXLogRecPtrGet(&GistPageGetOpaque(page)->nsn)) +#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(&GistPageGetOpaque(page)->nsn, val)) /* diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h index 52989e5e535..d52fefd4aa0 100644 --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -91,23 +91,45 @@ 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 the + * endianess because the field used to be stored as two 32-bit values. When + * reading and writing the LSN we need to convert between the two formats. + * + * We are careful to try to treat the LSN as a single uint64 so callers like + * BufferGetLSNAtomic() can be sure there are no torn reads or writes. */ -typedef struct +typedef uint64 PageXLogRecPtr; + +#ifdef WORDS_BIGENDIAN + +static inline XLogRecPtr +PageXLogRecPtrGet(const PageXLogRecPtr *val) +{ + return *val; +} + +static inline void +PageXLogRecPtrSet(PageXLogRecPtr *ptr, XLogRecPtr lsn) { - uint32 xlogid; /* high bits */ - uint32 xrecoff; /* low bits */ -} PageXLogRecPtr; + *ptr = lsn; +} + +#else static inline XLogRecPtr -PageXLogRecPtrGet(PageXLogRecPtr val) +PageXLogRecPtrGet(const volatile PageXLogRecPtr *val) +{ + PageXLogRecPtr tmp = *val; + return (tmp << 32) | (tmp >> 32); +} + +static inline void +PageXLogRecPtrSet(volatile PageXLogRecPtr *ptr, XLogRecPtr lsn) { - return (uint64) val.xlogid << 32 | val.xrecoff; + *ptr = (lsn << 32) | (lsn >> 32); } -#define PageXLogRecPtrSet(ptr, lsn) \ - ((ptr).xlogid = (uint32) ((lsn) >> 32), (ptr).xrecoff = (uint32) (lsn)) +#endif /* * disk page organization @@ -385,12 +407,12 @@ PageGetMaxOffsetNumber(const PageData *page) static inline XLogRecPtr PageGetLSN(const PageData *page) { - return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn); + return PageXLogRecPtrGet(&((const PageHeaderData *) page)->pd_lsn); } static inline void PageSetLSN(Page page, XLogRecPtr lsn) { - PageXLogRecPtrSet(((PageHeader) page)->pd_lsn, lsn); + PageXLogRecPtrSet(&((PageHeader) page)->pd_lsn, lsn); } static inline bool -- 2.47.3
