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