Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Aug 5, 2014 at 9:21 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> 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.
> 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'm rather concerned by this cavalier assumption that we can protect
fields a,b,c with one lock and fields x,y,z in the same struct with some
other lock.
A minimum requirement for that to work safely at all is that the fields
are of atomically fetchable/storable widths; which might be okay here
but it's a restriction that bears thinking about (and documenting).
But quite aside from safety, the fields are almost certainly going to
be in the same cache line which means contention between processes that
are trying to fetch or store them concurrently. For a patch whose sole
excuse for existence is to improve performance, that should be a very
scary concern.
(And yes, I realize these issues already affect the freelist. Perhaps
that's part of the reason we have performance issues with it.)
regards, tom lane
