Hi, On 2026-03-11 01:43:02 +0100, Tomas Vondra wrote: > >>> 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. > > > > I guess we are now reading 8 bytes as one. > > > > Not sure I understand. Are you saying it's OK or not?
I mean that this adds an 8 byte read, which alignment picky hardware could complain about, in theory. So I guess I kinda see the point of the change now. > >> 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)? > > > > It's definitely suboptimal. We should use something C11's > > atomic_load()/atomic_store(). But we can't rely on that yet (it's not > > enabled > > without extra flags in at least msvc). > > > > The volatile does prevent the compiler from just doing a read/write twice or > > never, just because it'd be nicer for code generation. But it doesn't > > actually guarantee that the right instructions for reading writing are > > generated :( > > > > That sounds a bit scary, TBH. Indeed. It's how atomic reads / writes have worked for quite a while though, so I think we'll just have to live with it until we can rely on C11 atomics on msvc. > From 794fb844266dcd8d97217da09b61c5c9cafc01d7 Mon Sep 17 00:00:00 2001 > From: Andreas Karlsson <[email protected]> > Date: Fri, 21 Nov 2025 10:15:06 +0100 > Subject: [PATCH v5] 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. I'd add a note explaining why heapfuncs is updated. > /* > * disk page organization > @@ -385,12 +411,13 @@ PageGetMaxOffsetNumber(const PageData *page) > static inline XLogRecPtr > PageGetLSN(const PageData *page) > { > - return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn); > + return PageXLogRecPtrGet(&((const volatile PageHeaderData *) > page)->pd_lsn); > } I don't think this volatile is needed, the one in PageXLogRecPtrGet's declaration should do the work. Greetings, Andres Freund
