Re: Atomic ops for unlogged LSN

Hi,

On 2023-05-24 08:22:08 -0400, Robert Haas wrote:
> On Tue, May 23, 2023 at 6:26 PM Michael Paquier <michael(at)paquier(dot)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