On Tue, Aug 26, 2014 at 11:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.)
False sharing is certainly a concern that has crossed my mine while
looking at Amit's work, but the performance numbers he's posted
upthread are stellar. Maybe we can squeeze some additional
performance out of this by padding out the cache lines, but it's
probably minor compared to the gains he's already seeing. I think we
should focus on trying to lock in those gains, and then we can
consider what further things we may want to do after that. If it
turns out that structure-padding is among those things, that's easy
enough to do as a separate patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
