public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andreas Karlsson <[email protected]>
To: Andres Freund <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations
Date: Wed, 14 Jan 2026 07:31:40 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <4e3gzrvqdrgw2sds3bnqylgbuq63j3ebtcgxomogkia4sqmhir@xmpndr5ufjmq>
References: <[email protected]>
<4e3gzrvqdrgw2sds3bnqylgbuq63j3ebtcgxomogkia4sqmhir@xmpndr5ufjmq>
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
Attachments:
[text/x-patch] v2-0001-Do-not-lock-in-BufferGetLSNAtomic-on-archs-with-8.patch (5.2K, ../[email protected]/2-v2-0001-Do-not-lock-in-BufferGetLSNAtomic-on-archs-with-8.patch)
download | inline diff:
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
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Remove header lock BufferGetLSNAtomic() on architectures with 64 bit atomic operations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox