Greetings,
* Nathan Bossart (nathandbossart@gmail.com) wrote:
> On Thu, May 25, 2023 at 07:41:21AM +0900, Michael Paquier wrote:
> > On Wed, May 24, 2023 at 02:49:58PM -0700, Andres Freund wrote:
> >> So we indeed loose some "barrier strength" - but I think that's fine. For one,
> >> it's a debugging-only value.
>
> Is it? I see uses in GiST indexing (62401db), so it's not immediately
> obvious to me how it is debugging-only. If it is, then I think this patch
> ought to clearly document it so that nobody else tries to use it for
> non-debugging-only stuff.
I don't see it as a debugging value. I'm not sure where that came
from..? We do use it in places and if anything, I expect it to be used
more, not less, in the future as a persistant generally increasing
value (could go backwards on a crash-restart but in such case all
unlogged tables are truncated).
> >> 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.
>
> My concern would be whether GetFakeLSNForUnloggedRel or CreateCheckPoint
> might see an old value of unloggedLSN. From the following note in
> README.barrier, it sounds like this would be a problem even if we ensured
> full barrier semantics:
>
> 3. No ordering guarantees. While memory barriers ensure that any given
> process performs loads and stores to shared memory in order, they don't
> guarantee synchronization. In the queue example above, we can use memory
> barriers to be sure that readers won't see garbage, but there's nothing to
> say whether a given reader will run before or after a given writer. If this
> matters in a given situation, some other mechanism must be used instead of
> or in addition to memory barriers.
>
> IIUC we know that shared memory accesses cannot be reordered to precede
> aquisition or follow release of a spinlock (thanks to 0709b7e), which is
> why this isn't a problem in the current implementation.
The concern around unlogged LSN, imv anyway, is less about shared memory
access and making sure that all callers understand that it can move
backwards on a crash/restart. I don't think that's an issue for current
users but we just need to make sure to try and comment sufficiently
regarding that such that new users understand that about this particular
value.
Thanks,
Stephen