Thread: Reducing ClogControlLock contention

ClogControlLock contention is high at commit time. This appears to be due to the fact that ClogControlLock is acquired in Exclusive mode prior to marking commit, which then gets starved by backends running TransactionIdGetStatus().

Proposal for improving this is to acquire the ClogControlLock in Shared mode, if possible.

This is safe because people checking visibility of an xid must always run TransactionIdIsInProgress() first to avoid race conditions, which will always return true for the transaction we are currently committing. As a result, we never get concurrent access to the same bits in clog, which would require a barrier.

Two concurrent writers might access the same word concurrently, so we protect against that with a new CommitLock. We could partition that by pageno also, if needed.

On Tue, Jun 30, 2015 at 4:02 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

ClogControlLock contention is high at commit time. This appears to be due to the fact that ClogControlLock is acquired in Exclusive mode prior to marking commit, which then gets starved by backends running TransactionIdGetStatus().

Proposal for improving this is to acquire the ClogControlLock in Shared mode, if possible.

This is safe because people checking visibility of an xid must always run TransactionIdIsInProgress() first to avoid race conditions, which will always return true for the transaction we are currently committing. As a result, we never get concurrent access to the same bits in clog, which would require a barrier.

Two concurrent writers might access the same word concurrently, so we protect against that with a new CommitLock. We could partition that by pageno also, if needed.

Could it be possible to see some performance numbers? For example with a simple pgbench script doing a bunch of tiny transactions, with many concurrent sessions (perhaps hundreds).

This contention is masked by contention elsewhere, e.g. ProcArrayLock, so the need for testing here should come once other patches ahead of this are in.

On Tue, Jun 30, 2015 at 12:32 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> ClogControlLock contention is high at commit time. This appears to be due to the fact that ClogControlLock is acquired in Exclusive mode prior to marking commit, which then gets starved by backends running TransactionIdGetStatus().
>
> Proposal for improving this is to acquire the ClogControlLock in Shared mode, if possible.
>

This approach looks good way for avoiding the contention around

ClogControlLock.  Few things that occurred to me while looking at

patch are that

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.

b. SimpleLruReadPage_ReadOnly() - This API's is meant mainly for

read-access of page and the description also says the same, but now

we want to use it for updating page as well. It might be better to invent

similar new API or at the very least modify it's description.

> Two concurrent writers might access the same word concurrently, so we protect against that with a new CommitLock. We could partition that by pageno also, if needed.
>

I think it will be better to partition it or use it in some other way to avoid

two concurrent writers block at it, however if you want to first see the

test results with this, then that is also okay.

Overall the idea seems good to pursue, however I have slight feeling

that using 2 LWLocks (CLOGControlLock in shared mode and new

CommitLock in Exclusive mode) to set the transaction information

is somewhat odd, but I could not see any problem with it.

On Wed, Jul 1, 2015 at 1:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 1 July 2015 at 09:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>> I think it will be better to partition it or use it in some other way to avoid
>> two concurrent writers block at it, however if you want to first see the
>> test results with this, then that is also okay.
>
>
> Many updates would be on same page, so partitioning it would need to be at least 4-way to be worth doing. Maybe we could stripe into 512 bye pages.

>

Sure, it makes sense to try that way, once you have that ready, I can

try this out along with ProcArrayLock patch to see the impact.

On 2015-07-01 09:08:11 +0100, Simon Riggs wrote:
> On 1 July 2015 at 09:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> > On Tue, Jun 30, 2015 at 12:32 PM, Simon Riggs <simon@2ndquadrant.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.

On Wed, Jul 1, 2015 at 3:49 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 1 July 2015 at 11:14, Andres Freund <andres@anarazel.de> wrote:
>>
>> On 2015-07-01 09:08:11 +0100, Simon Riggs wrote:
>> > On 1 July 2015 at 09:00, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >
>> > > On Tue, Jun 30, 2015 at 12:32 PM, Simon Riggs <simon@2ndquadrant.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.

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.

On 11 August 2015 at 10:55, Amit Kapila <amit.kapila16@gmail.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. 

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.

One more point here why do we need CommitLock before calling

SimpleLruReadPage_ReadOnly() in the patch and if it is not required,

then can we use LWLockAcquire(shared->buffer_locks[slotno], LW_EXCLUSIVE);

instead of CommitLock?

On Tue, Aug 11, 2015 at 3:44 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 11 August 2015 at 10:55, Amit Kapila <amit.kapila16@gmail.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.

On Tue, Aug 11, 2015 at 7:27 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 11 August 2015 at 14:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
>  
>>
>> One more point here why do we need CommitLock before calling
>> SimpleLruReadPage_ReadOnly() in the patch and if it is not required,
>> then can we use LWLockAcquire(shared->buffer_locks[slotno], LW_EXCLUSIVE);
>> instead of CommitLock?
>
>
> That prevents read only access, not just commits, so that isn't a better suggestion.

read only access of what (clog page?)?

Here we are mainly doing three operations read clog page, write transaction status

on clog page and update shared control state.  So basically two resources are

involved clog page and shared control state, so which one of those you are talking?

Apart from above, in below code, it is assumed that we have exclusive lock on

clog page which we don't in the proposed patch as some one can read the

same page while we are modifying it. In current code, this assumption is valid

because during Write we take CLogControlLock in Exclusive mode and while

Reading we take the same in Shared mode.

On 2015-06-30 08:02:25 +0100, Simon Riggs wrote:
> Proposal for improving this is to acquire the ClogControlLock in Shared
> mode, if possible.

I find that rather scary. That requires for all read and write paths in
clog.c and slru.c to only ever read memory locations once. Otherwise
those reads may not get the same results. That'd mean we'd need to add
indirections via volatile to all reads/writes. It also requires that we
never read in 4 byte quantities.

> This is safe because people checking visibility of an xid must always run
> TransactionIdIsInProgress() first to avoid race conditions, which will
> always return true for the transaction we are currently committing. As a
> result, we never get concurrent access to the same bits in clog, which
> would require a barrier.

I don't think that's really sufficient. There's a bunch of callers do
lookups without such checks, e.g. in heapam.c. It might be safe due to
other circumstances, but at the very least this is a significant but
sublte API revision.

TransactionIdSetPageStatus() calls TransactionIdSetStatusBit(), which
writes an 8 byte variable (the lsn). That's not safe.

A patch like this will need far more changes. Every read and write from
a page has to be guaranteed to only be done once, otherwise you can get
'effectively torn' values.

That is, you can't just do
static void
TransactionIdSetStatusBit(TransactionId xid, XidStatus status, XLogRecPtr lsn, int slotno)
...
        char       *byteptr;
        char        byteval;
...
        /* note this assumes exclusive access to the clog page */
        byteval = *byteptr;
        byteval &= ~(((1 << CLOG_BITS_PER_XACT) - 1) << bshift);
        byteval |= (status << bshift);
        *byteptr = byteval;
...

the compiler is entirely free to "optimize" away the byteval variable
and do all these masking operations on memory! It can intermittenly
write temporary values to byteval, because e.g. the register pressure is
too high.

To actually rely on single-copy-atomicity you have to enforce that these
reads/writes can only happen. Leavout out some possible macro
indirection that'd have to look like
        byteval = (volatile char *) byteptr;
        ...
        *(volatile char *) byteptr = byteval;
some for TransactionIdGetStatus(), without the write side obviously.

On 22 August 2015 at 15:14, Andres Freund <andres@anarazel.de> wrote:

TransactionIdSetPageStatus() calls TransactionIdSetStatusBit(), which
writes an 8 byte variable (the lsn). That's not safe.

Agreed, thanks for spotting that.

I see this as the biggest problem to overcome with this patch.

On Tue, Aug 25, 2015 at 2:21 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 22 August 2015 at 15:14, Andres Freund <andres@anarazel.de> wrote:

 

TransactionIdSetPageStatus() calls TransactionIdSetStatusBit(), which
writes an 8 byte variable (the lsn). That's not safe.

Agreed, thanks for spotting that.

I see this as the biggest problem to overcome with this patch.

How about using 64bit atomics or spinlock to protect this variable?

On 08/31/2015 07:34 AM, Amit Kapila wrote:

I have updated the patch (attached with mail) to show
you what I have in mind.

I havn't been able to get a successful run with _v5 using pgbench.

TransactionIdSetStatusBit assumes an exclusive lock on CLogControlLock when called, but that part is removed from TransactionIdSetPageStatus now.

I tried an

  if (!LWLockHeldByMe(CLogControlLock))
  {
      LWLockAcquire(CLogControlLock, LW_EXCLUSIVE);
      mode = LW_EXCLUSIVE;
  }

approach, but didn't get further.

Plus that probably isn't the best way, since we will traverse all LWLock's,

TransactionIdSetPageStatus() calls TransactionIdSetStatusBit(), which
writes an 8 byte variable (the lsn). That's not safe.