Re: Reducing ClogControlLock contention

On 11 August 2015 at 11:39, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Tue, Aug 11, 2015 at 3:44 PM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
>
>> On 11 August 2015 at 10:55, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>
>>> > What "tricks" are being used??
>>> >
>>> > Please explain why taking 2 locks is bad here, yet works fine
>>> elsewhere.
>>> >
>>>
>>> One thing that could be risky in this new scheme of locking
>>> is that now in functions TransactionIdSetPageStatus and
>>> TransactionIdSetStatusBit, we modify slru's shared state with Control
>>> Lock
>>> in Shared mode whereas I think it is mandated in the code that those
>>> should be modified with ControlLock in Exlusive mode. This could have
>>> some repercussions.
>>>
>>
>> Do you know of any? This is a technical forum, so if we see a problem we
>> say what it is, and if we don't, that's usually classed as a positive point
>> in a code review.
>>
>
> One of the main reason of saying this is that it is written in File
> level comments in slru.c that for accessing (examine or modify)
> the shared state, Control lock *must* be held in Exclusive mode
> except in function SimpleLruReadPage_ReadOnly(). So, whereas
> I agree that I should think more about if there is any breakage due
> to patch, but I don't find any explanation either in your e-mail or in
> patch why it is safe to modify the state after patch when it was not
> before. If you think it is safe, then atleast modify comments in
> slru.c.
>

"except"...

I explained that a reader will never be reading data that is concurrently
changed by a writer, so it was safe to break the general rule for this
specific case only.

Yes, I will modify comments in the patch.

> Another thing is that in this flow, with patch there will be three locks
>>> (we take per-buffer locks before doing I/O) that will get
>>> involved rather than
>>> two, so one effect of this patch will be that currently while doing I/O,
>>> concurrent committers will be allowed to proceed as we
>>> release ControlLock
>>> before doing the same whereas with Patch, they will not be allowed as
>>> they
>>> are blocked by CommitLock. Now may be this scenario is less common and
>>> doesn't matter much if the patch improves the more common scenario,
>>> however this is an indication of what Andres tries to highlight that
>>> having more
>>> locks for this might make patch more complicated.
>>>
>>
>> It's easy to stripe the CommitLock in that case, if it is a problem.
>>
>
> Sure, I think other places in code that take both the other locks also
> needs to be checked for updation.
>

Not sure what that means, but there are no other places calling CommitLock

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services