Noah Misch <noah@leadboat.com> writes:
> On Sun, Jun 07, 2020 at 12:23:35AM -0400, Tom Lane wrote:
>> ... But on thinking more about this, it seems like
>> generic.h's version of pg_atomic_init_u64_impl is just fundamentally
>> misguided. Why isn't it simply assigning the value with an ordinary
>> unlocked write? By definition, we had better not be using this function
>> in any circumstance where there might be conflicting accesses
> Does something guarantee the write will be globally-visible by the time the
> first concurrent accessor shows up? (If not, one could (a) do an unlocked
> ptr->value=0, then the atomic write, or (b) revert and improve the
> suppression.) I don't doubt it's fine for the ways PostgreSQL uses atomics
> today, which generally initialize an atomic before the concurrent-accessor
> processes even exist.
Perhaps it'd be worth putting a memory barrier at the end of the _init
function(s)? As you say, this is hypothetical right now, but that'd be
a cheap improvement.
>> if a simple assignment *isn't* good enough, then surely the spinlock
>> version in atomics.c is 100% broken.
> Are you saying it would imply a bug in atomics.c pg_atomic_init_u64_impl(),
> pg_atomic_compare_exchange_u64_impl(), or pg_atomic_fetch_add_u64_impl()? Can
> you explain that more?
My point was that doing SpinLockInit while somebody else is already trying
to acquire or release the spinlock is not going to work out well. So
there has to be a really clear boundary between "initialization" mode
and "use" mode; which is more or less the same point you make above.
In practice, if that line is so fine that we need a memory sync operation
to enforce it, things are probably broken anyhow.
regards, tom lane