Hi, On 2023-01-10 13:11:35 -0500, Robert Haas wrote: > On Tue, Jan 10, 2023 at 12:40 PM Andres Freund <and...@anarazel.de> wrote: > > > I think. `expected = originalVictim + 1;` line should be in while loop > > > (before acquiring spin lock) so that, even in the case above, expected > > > variable is incremented for each loop and CAS operation will be successful > > > at some point. > > > Is my understanding correct? If so, I will send PR for fixing this issue. > > > > Yes, I think your understanding might be correct. Interesting that this > > apparently has never occurred. > > Doesn't pg_atomic_compare_exchange_u32 set expected if it fails?
Indeed, so there's no problem. I wonder if we should make ->nextVictimBuffer a 64bit atomic. At the time the changes went in we didn't (or rather, couldn't) rely on it, but these days we could. I think with a 64bit number we could get rid of ->completePasses and just infer it from ->nextVictimBuffer/NBuffers, removing th need for the spinlock. It's very unlikely that 64bit would ever wrap, and even if, it'd just be a small inaccuracy in the assumed number of passes. OTOH, it's doubtful the overflow handling / the spinlock matters performance wise. Greetings, Andres Freund