Hi,
On 2023-11-07 00:57:32 +0000, John Morris wrote:
> I found the comment about cache coherency a bit confusing. We are dealing
> with a single address, so there should be no memory ordering or coherency
> issues. (Did I misunderstand?) I see it more as a race condition.
IMO cache coherency covers the value a single variable has in different
threads / processes.
In fact, the only reason there effectively is a guarantee that you're not
seeing an outdated unloggedLSN value during shutdown checkpoints, even without
the spinlock or full barrier atomic op, is that the LWLockAcquire(), a few
lines above this, would prevent both the compiler and CPU from moving the read
of unloggedLSN to much earlier. Obviously that lwlock has a different
address...
If the patch just had done the minimal conversion, it'd already have been
committed... Even if there'd be a performance reason to get rid of the memory
barrier around reading unloggedLSN in CreateCheckPoint(), I'd do the
conversion in a separate commit.
Greetings,
Andres Freund