Re: Reducing ClogControlLock contention

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.

> 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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com