Hi, Thank you for your quick reply I misunderstood the logic of pg_atomic_compare_exchange_u32, so the loop cannot be infinite.
> 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. I'm not sure why 64 bit was not used at the time, so I'm concerned about it. but, except for it, you have a point and I completely agree with you. as you have said, we should use 64 bit whose higher-order 32 bit indicates completePasses, and should remove spinlock. maybe we don't have to exceptionally worry about the overflow here mainly because, even now, the completePasses can overflow and the possibility of overflow may not be so different so that the 64 bit atomic operation is better. if overflow would happen, passes_delta variable in the function called by bgwriter would be negative high value and it would lead to the failure of assert. (the code is below https://github.com/postgres/postgres/blob/d9d873bac67047cfacc9f5ef96ee488f2cb0f1c3/src/backend/storage/buffer/bufmgr.c#L2298-L2303 Do you send patch for the replacement with 64 bit? If you don't mind, I would like to send patch. ( or is there some procedure before sending patch? Thanks hayato 2023年1月11日(水) 3:59 Andres Freund <and...@anarazel.de>: > 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 >
