Reducing ClogControlLock contention

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

On 30 June 2015 at 08:13, Michael Paquier <michael.paquier@gmail.com> wrote:

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

I'm more interested to see if people think it is safe.

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.

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

On 30 June 2015 at 08:22, Simon Riggs <simon@2ndquadrant.com> wrote:

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.

Let me explain more clearly.

Andres' patch to cache snapshots and reduce ProcArrayLock was interesting,
but not initially compelling. We now have a solution that commits in
batches, which will reduce the number of times the ProcArray changes - this
will heighten the benefit from Andres' snapshot cache patch.

So the order of testing/commit should be

Queued commit patch
ProcArray cache patch
Clog shared commit patch (this one)

I didn't hear recent mention of Robert's chash patch, but IIRC it was
effective and so we hope to see it again soon also.

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

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

On 30 June 2015 at 08:13, Michael Paquier <michael.paquier@gmail.com>

wrote:

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

I'm more interested to see if people think it is safe.

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.

Exactly and other lock that can mask this improvement is WALWriteLock,
but for that we can take the performance data with synchronous_commit
off mode.

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

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.

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

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

LWLock API code is already too complex, so -1 for more changes there

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.

Agreed, perhaps SimpleLruReadPage_Optimistic()

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.

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.

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.

Perhaps call it the CommitSerializationLock would help. There are already
locks that are held only in Exclusive mode.

Locking two separate resources at same time is common in other code.

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

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.

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

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.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1 July 2015 at 11:11, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

Seems sensible to measure what the new point of contention is with both
before doing anything further.

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

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.

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

On 2015-07-01 11:19:40 +0100, Simon Riggs wrote:

What "tricks" are being used??

Please explain why taking 2 locks is bad here, yet works fine elsewhere.

I didn't say anything about 'bad'. It's more complicated than one
lock. Suddenly you have to care about lock ordering and such. The
algorithms for ensuring correctness gets more complicated.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Jul 1, 2015 at 6:21 AM, Andres Freund <andres@anarazel.de> wrote:

On 2015-07-01 11:19:40 +0100, Simon Riggs wrote:

What "tricks" are being used??

Please explain why taking 2 locks is bad here, yet works fine elsewhere.

I didn't say anything about 'bad'. It's more complicated than one
lock. Suddenly you have to care about lock ordering and such. The
algorithms for ensuring correctness gets more complicated.

Taking two locks might also be more expensive than just taking one. I
suppose benchmarking will reveal whether there is an issue there.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

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

On 11 August 2015 at 10:55, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

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/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

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

On Tue, Aug 11, 2015 at 4:09 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

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

wrote:

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.

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?

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

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.

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

On 11 August 2015 at 11:39, Amit Kapila <amit.kapila16@gmail.com> wrote:

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.

"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/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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.

TransactionIdSetStatusBit()
{
..
/* note this assumes exclusive access to the clog page */
byteval = *byteptr;
byteval &= ~(((1 << CLOG_BITS_PER_XACT) - 1) << bshift);
byteval |= (status << bshift);
*byteptr = byteval;
..
}

Now even if this is a problem, I think we can solve it with some more lower
level lock or may be with atomic operation, but I have mentioned it to check
your opinion on the same.

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