On Tue, Aug 15, 2023 at 12:29:24PM +0200, Michail Nikolaev wrote:
>> What sort of benefits do you see from this patch? It might be worthwhile
>> in itself to remove spinlocks when possible, but IME it's much easier to
>> justify such changes when there is a tangible benefit we can point to.
>
> Oh, it is not an easy question :)
>
> The answer, probably, looks like this:
> 1) performance benefits of spin lock acquire removing in
> KnownAssignedXidsGetOldestXmin and KnownAssignedXidsSearch
> 2) it is closing 13-year-old tech depth
>
> But in reality, it is not easy to measure performance improvement
> consistently for this change.
Okay. Elsewhere, it seems like folks are fine with patches that reduce
shared memory space via atomics or barriers even if there's no immediate
benefit [0], so I think it's fine to proceed.
>> Are the assignments in question guaranteed to be atomic? IIUC we assume
>> that aligned 4-byte loads/stores are atomic, so we should be okay as long
>> as we aren't handling anything larger.
>
> Yes, 4-bytes assignment are atomic, locking is used to ensure memory
> write ordering in this place.
Yeah, it looks like both the values that are protected by
known_assigned_xids_lck are integers, so this should be okay. One
remaining question I have is whether it is okay if we see an updated value
for one of the head/tail variables but not the other. It looks like the
tail variable is only updated with ProcArrayLock held exclusively, which
IIUC wouldn't prevent such mismatches even today, since we use a separate
spinlock for reading them in some cases.
[0] https://postgr.es/m/20230524214958.mt6f5xokpumvnrio%40awork3.anarazel.de
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
