On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote: > I was a bit confused by Michael's comment as well, due to the section of code > quoted. But he does have a point: pg_atomic_read_u32() does indeed *not* have > barrier semantics (it'd be way more expensive), and the patch does contain > this hunk:
Thanks for the correction. The part about _add was incorrect. > So we indeed loose some "barrier strength" - but I think that's fine. For one, > it's a debugging-only value. But more importantly, I don't see what reordering > the barrier could prevent - a barrier is useful for things like sequencing two > memory accesses to happen in the intended order - but unloggedLSN doesn't > interact with another variable that's accessed within the ControlFileLock > afaict. This stuff is usually tricky enough that I am never completely sure whether it is fine without reading again README.barrier, which is where unloggedLSN is saved in the control file under its LWLock. Something that I find confusing in the patch is that it does not document the reason why this is OK. -- Michael
signature.asc
Description: PGP signature
