Re: StrategyGetBuffer optimization, take 2

On 2013-08-07 09:40:24 -0500, Merlin Moncure wrote:
> > I don't think the unlocked increment of nextVictimBuffer is a good idea
> > though. nextVictimBuffer jumping over NBuffers under concurrency seems
> > like a recipe for disaster to me. At the very, very least it will need a
> > good wad of comments explaining what it means and how you're allowed to
> > use it. The current way will lead to at least bgwriter accessing a
> > nonexistant/out of bounds buffer via StrategySyncStart().
> > Possibly it won't even save that much, it might just increase the
> > contention on the buffer header spinlock's cacheline.
>
> I agree; at least then it's not unambiguously better. if you (in
> effect) swap all contention on allocation from a lwlock to a spinlock
> it's not clear if you're improving things; it would have to be proven
> and I'm trying to keep things simple.

I think converting it to a spinlock actually is a good idea, you just
need to expand the scope a bit.

> Attached is a scaled down version of the patch that keeps the freelist
> lock but still removes the spinlock during the clock sweep. This
> still hits the major objectives of reducing the chance of scheduling
> out while holding the BufFreelistLock and mitigating the worst case
> impact of doing so if it does happen.

FWIW, I am not convinced this is the trigger for the problems you're
seing. It's a good idea nonetheless though.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services