Andres Freund <andres@anarazel.de> writes:
> On 2020-06-03 14:19:45 -0400, Tom Lane wrote:
>> This seems to me to be very bad code.
> I think straight out prohibiting use of atomics inside a spinlock will
> lead to more complicated and slower code, rather than than improving
> anything. I'm to blame for the ClockSweepTick() case, and I don't really
> see how we could avoid the atomic-while-spinlocked without relying on
> 64bit atomics, which are emulated on more platforms, and without having
> a more complicated retry loop.
Well, if you don't want to touch freelist.c then there is no point
worrying about what pg_stat_get_wal_receiver is doing. But having
now studied that freelist.c code, I don't like it one bit. It's
overly complicated and not very accurately commented, making it
*really* hard to convince oneself that it's not buggy.
I think a good case could be made for ripping out what's there now
and just redefining nextVictimBuffer as a pg_atomic_uint64 that we
never reset (ie, make its comment actually true). Then ClockSweepTick
reduces to
pg_atomic_fetch_add_u64(&StrategyControl->nextVictimBuffer, 1) % NBuffers
and when we want to know how many cycles have been completed, we just
divide the counter by NBuffers. Now admittedly, this is relying on both
pg_atomic_fetch_add_u64 being fast and int64 division being fast ... but
to throw your own argument back at you, do we really care anymore about
performance on machines where those things aren't true? Furthermore,
since all this is happening in a code path that's going to lead to I/O,
I'm not exactly convinced that a few nanoseconds matter anyway.
regards, tom lane