Hi,
Andres pointed out this possible optimization on Discord so I hacked up
a quick patch which avoids taking a lock when reading the LSN from a
page on architectures where we can be sure to not get a torn value. It
is always nice to remove a lock from a reasonably hot code path.
I thought about using our functions for atomics but did not do so since
I did not want to introduce any extra overhead on platforms which do not
support 64-bit atomic operations.
I decided to just remove the struct to make the code simpler and more
consistent but I can also see an argument for keeping it to get some
degree of type safety.
I have not properly benchmarked it yet but plan to do so when I am back
from my vacation.
I have also included a cleanup patch where I change a macro into an
inline function which I think improves code readability. Feel free to
ignore that one if you want.
--
Andreas
Percona
From dbf345ac5a857a11801d07291ecd71eb224050af Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <[email protected]>
Date: Fri, 21 Nov 2025 10:15:06 +0100
Subject: [PATCH v1 1/2] 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/storage/buffer/bufmgr.c | 12 ++++++++---
src/include/storage/bufpage.h | 33 +++++++++++++++++++++--------
2 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 327ddb7adc8..34c4c4bd925 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4491,13 +4491,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;
@@ -4518,6 +4523,7 @@ BufferGetLSNAtomic(Buffer buffer)
UnlockBufHdr(bufHdr);
return lsn;
+#endif
}
/* ---------------------------------------------------------------------
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index abc2cf2a020..1b54967c70c 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -91,23 +91,38 @@ 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
-{
- uint32 xlogid; /* high bits */
- uint32 xrecoff; /* low bits */
-} PageXLogRecPtr;
+typedef uint64 PageXLogRecPtr;
+
+#ifdef WORDS_BIGENDIAN
static inline XLogRecPtr
PageXLogRecPtrGet(PageXLogRecPtr val)
{
- return (uint64) val.xlogid << 32 | val.xrecoff;
+ return val;
}
#define PageXLogRecPtrSet(ptr, lsn) \
- ((ptr).xlogid = (uint32) ((lsn) >> 32), (ptr).xrecoff = (uint32) (lsn))
+ ((ptr) = (lsn))
+
+#else
+
+static inline XLogRecPtr
+PageXLogRecPtrGet(volatile PageXLogRecPtr val)
+{
+ return (val << 32) | (val >> 32);
+}
+
+#define PageXLogRecPtrSet(ptr, lsn) \
+ ((ptr) = ((lsn) << 32) | ((lsn) >> 32))
+
+#endif
/*
* disk page organization
--
2.47.3
From fbf115b1c567183ef16961f08e7cf978235c8c33 Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <[email protected]>
Date: Fri, 21 Nov 2025 14:34:03 +0100
Subject: [PATCH v1 2/2] Convert PageXLogRecPtrSet() from macro to inline
function
This makes the code a bit more consistent and clean while providing a
tiny bit of extra type safety.
---
src/backend/access/common/bufmask.c | 2 +-
src/include/access/gist.h | 2 +-
src/include/storage/bufpage.h | 16 +++++++++++-----
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/common/bufmask.c b/src/backend/access/common/bufmask.c
index bb260cffa68..fa6b5099226 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/include/access/gist.h b/src/include/access/gist.h
index b3f4e02cbfd..2777b5de6f3 100644
--- a/src/include/access/gist.h
+++ b/src/include/access/gist.h
@@ -187,7 +187,7 @@ typedef struct GISTENTRY
#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 GistPageSetNSN(page, val) ( PageXLogRecPtrSet(&GistPageGetOpaque(page)->nsn, val))
/*
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 1b54967c70c..331d471bf97 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -108,8 +108,11 @@ PageXLogRecPtrGet(PageXLogRecPtr val)
return val;
}
-#define PageXLogRecPtrSet(ptr, lsn) \
- ((ptr) = (lsn))
+static inline void
+PageXLogRecPtrSet(PageXLogRecPtr *ptr, XLogRecPtr lsn)
+{
+ *ptr = lsn;
+}
#else
@@ -119,8 +122,11 @@ PageXLogRecPtrGet(volatile PageXLogRecPtr val)
return (val << 32) | (val >> 32);
}
-#define PageXLogRecPtrSet(ptr, lsn) \
- ((ptr) = ((lsn) << 32) | ((lsn) >> 32))
+static inline void
+PageXLogRecPtrSet(PageXLogRecPtr *ptr, XLogRecPtr lsn)
+{
+ *ptr = (lsn << 32) | (lsn >> 32);
+}
#endif
@@ -405,7 +411,7 @@ PageGetLSN(const PageData *page)
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
