On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <
robertmhaas@gmail.com> wrote:
>
> Incidentally, while I generally think your changes to the locking regimen in StrategyGetBuffer() are going in the right direction, they need significant cleanup. Your patch adds two new spinlocks, freelist_lck and victimbuf_lck, that mostly but not-quite replace BufFreelistLock, and you've now got StrategyGetBuffer() running with no lock at all when accessing some things that used to be protected by BufFreelistLock; specifically, you're doing StrategyControl->numBufferAllocs++ and SetLatch(StrategyControl->bgwriterLatch) without any locking. That's not OK. I think you should get rid of BufFreelistLock completely and just decide that freelist_lck will protect everything the freeNext links, plus everything in StrategyControl except for nextVictimBuffer. victimbuf_lck will protect nextVictimBuffer and nothing else.
>
> Then, in StrategyGetBuffer, acquire the freelist_lck at the point where the LWLock is acquired today. Increment StrategyControl->numBufferAllocs; save the values of StrategyControl->bgwriterLatch; pop a buffer off the freelist if there is one, saving its identity. Release the spinlock. Then, set the bgwriterLatch if needed. In the first loop, first check whether the buffer we previously popped from the freelist is pinned or has a non-zero usage count and return it if not, holding the buffer header lock. Otherwise, reacquire the spinlock just long enough to pop a new potential victim and then loop around.
>
Today, while working on updating the patch to improve locking
I found that as now we are going to have a new process, we need
a separate latch in StrategyControl to wakeup that process.
Another point is I think it will be better to protect
StrategyControl->completePasses with victimbuf_lck rather than
freelist_lck, as when we are going to update it we will already be
holding the victimbuf_lck and it doesn't make much sense to release
the victimbuf_lck and reacquire freelist_lck to update it.
I thought it is better to mention about above points so that if you have
any different thoughts about it, then it is better to discuss them now
rather than after I take performance data with this locking protocol.
With Regards,
Amit Kapila.
EnterpriseDB:
http://www.enterprisedb.com
