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

On 3/10/26 21:41, Andres Freund wrote:
> 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 <andreas(at)proxel(dot)se> 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.
>

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.

AFAIK proper alignment is required to make the load atomic.

>> 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.
>

OK, WFM.

>
>> 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.
>

I agree. It'd be trivial to call it with plain XLogRecPtr (especially
now, with a function handling both directions).

>
>> -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.
>

I did that in v5 (I think). But at the same time I'm now rather confused
about the meaning of PG_HAVE_8BYTE_SINGLE_COPY_ATOMICITY. Because if a
well-aligned load/store of 8B can be implemented as two 32-bit reads
(with a "sequence point" in between), then how is that atomic?

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)?

>
>> 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 agree, I found that confusing too. In the attached v5 I went back to
the separate get/set functions from v3. I think that's better/clearer.

regards

--
Tomas Vondra