Re: Reducing ClogControlLock contention

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

> On Wed, Jul 1, 2015 at 3:49 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >
> > On 1 July 2015 at 11:14, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >>
> >> On 2015-07-01 09:08:11 +0100, Simon Riggs wrote:
> >> > On 1 July 2015 at 09:00, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >> >
> >> > > On Tue, Jun 30, 2015 at 12:32 PM, Simon Riggs <
> simon(at)2ndquadrant(dot)com>
> >> > > a. the semantics of new LWLock (CommitLock) introduced
> >> > > by patch seems to be different in the sense that it is just taken in
> >> > > Exclusive mode (and no Shared mode is required) as per your
> proposal. We
> >> > > could use existing LWLock APi's, but on the other hand we could even
> >> > > invent new LWLock API for this kind of locking.
> >> > >
> >> >
> >> > LWLock API code is already too complex, so -1 for more changes there
> >>
> >> I don't think that's a valid argument. It's better to have the
> >> complexity in one place (lwlock) than have rather similar complexity in
> >> several other places. The clog control lock is far from the only place
> >> that would benefit from tricks along these lines.
> >
> >
> > 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.

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

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