Hi, On 2023-05-24 08:22:08 -0400, Robert Haas wrote: > On Tue, May 23, 2023 at 6:26 PM Michael Paquier <mich...@paquier.xyz> wrote: > > Spinlocks provide a full memory barrier, which may not the case with > > add_u64() or read_u64(). Could memory ordering be a problem in these > > code paths? > > It could be a huge problem if what you say were true, but unless I'm > missing something, the comments in atomics.h say that it isn't. The > comments for the 64-bit functions say to look at the 32-bit functions, > and there it says: > > /* > * pg_atomic_add_fetch_u32 - atomically add to variable > * > * Returns the value of ptr after the arithmetic operation. > * > * Full barrier semantics. > */ > > Which is probably a good thing, because I'm not sure what good it > would be to have an operation that both reads and modifies an atomic > variable but has no barrier semantics.
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: > @@ -6784,9 +6775,7 @@ CreateCheckPoint(int flags) > * unused on non-shutdown checkpoints, but seems useful to store it > always > * for debugging purposes. > */ > - SpinLockAcquire(&XLogCtl->ulsn_lck); > - ControlFile->unloggedLSN = XLogCtl->unloggedLSN; > - SpinLockRelease(&XLogCtl->ulsn_lck); > + ControlFile->unloggedLSN = pg_atomic_read_u64(&XLogCtl->unloggedLSN); > > UpdateControlFile(); > LWLockRelease(ControlFileLock); 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. > That's not to say that I entirely understand the point of this patch. > It seems like a generally reasonable thing to do AFAICT but somehow I > wonder if there's more to the story here, because it doesn't feel like > it would be easy to measure the benefit of this patch in isolation. > That's not a reason to reject it, though, just something that makes me > curious. I doubt it's a meaningful, if even measurable win. But removing atomic ops and reducing shared memory space isn't a bad thing, even if there's no immediate benefit... Greetings, Andres Freund
