Re: Patch to improve performance of replay of AccessExclusiveLock

On 16 March 2017 at 13:31, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 8 March 2017 at 10:02, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
> wrote:
> > On 8 March 2017 at 01:13, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> Don't understand this. I'm talking about setting a flag on
> >> commit/abort WAL records, like the attached.
> >
> > There's nothing setting a flag in the attached. I only see the
> > checking of the flag.
>
> Yeh, it wasn't a full patch, just a way marker to the summit for you.
>
> >> We just need to track info so we can set the flag at EOXact and we're
> done.
> >
> > Do you have an idea where to store that information? I'd assume we'd
> > want to store something during LogAccessExclusiveLocks() and check
> > that in XactLogCommitRecord() and XactLogAbortRecord(). I don't see
> > anything existing which might help with that today. Do you think I
> > should introduce some new global variable for that? Or do you propose
> > calling GetRunningTransactionLocks() again while generating the
> > Commit/Abort record?
>
> Seemed easier to write it than explain further. Please see what you think.

Thanks. I had been looking for some struct to store the flag in. I'd not
considered just adding a new global variable.

I was in fact just working on this again earlier, and I had come up with
the attached.

I was just trying to verify if it was going to do the correct thing in
regards to subtransactions and I got a little confused at the comments at
the top of StandbyAcquireAccessExclusiveLock():

* We record the lock against the top-level xid, rather than individual
* subtransaction xids. This means AccessExclusiveLocks held by aborted
* subtransactions are not released as early as possible on standbys.

if this is true, and it seems to be per LogAccessExclusiveLock(),
does StandbyReleaseLockTree() need to release the locks for sub
transactions too?

This code:

for (i = 0; i < nsubxids; i++)
StandbyReleaseLocks(subxids[i]);

FWIW I tested the attached and saw that with my test cases I showed earlier
that StandbyReleaseLockTree() was down to 0.74% in the perf report. Likely
the gain will be even better if I ran a few more CPUs on small simple
transactions which are not taking Access Exclusive locks, so I agree this
is a better way forward than what I had in my original patch. Your patch
will have the same effect, so much better than the 1.74% I saw in my perf
report for the 1024 bucket hash table.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services