Re: Patch to improve performance of replay of AccessExclusiveLock

On 20 March 2017 at 08:31, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> On 18 March 2017 at 21:59, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> As Amit says, I don't see the gain from adding that to each xact state.
>>
>> I'd suggest refactoring my patch so that the existign
>> MyXactAccessedTempRel becomes MyXactFlags and we can just set a flag
>> in the two cases (temp rels and has-aels). That way we still have one
>> Global but it doesn't get any uglier than it already is.
>
> OK, now that I understand things a bit better, I've gone over your
> patch and changing things around that it combines the two variables
> into a flags variable which will be better suited for future
> expansion.
>
> I also fixed up the missing code from the Abort transaction. This is patch 0001

This looks good to go, will final check and apply. Thanks,

> 0002:
>
> I was thinking it might be a good idea to back patch the patch which
> removes the bogus code from StandbyReleaseLockTree(). Any users of
> sub-transactions are currently paying a price that they've no reason
> to, especially so when there's AELs held by other concurrent
> transactions. We already know this code is slow, having this mistake
> in there is not helping any.
>
> 0003:
>
> Is intended to be patched atop of 0002 (for master only) and revises
> this code further to remove the StandbyReleaseLockTree() function. To
> me it seems better to do this to clear up any confusion about what's
> going on here. This patch does close the door a little on tracking
> AELs on sub-xacts. I really think if we're going to do that, then it
> won't be that hard to put it back again. Seems better to be consistent
> here and not leave any code around that causes people to be confused
> about the same thing as I was confused about. This patch does get rid
> of StandbyReleaseLockTree() which isn't static. Are we likely to break
> any 3rd party code by doing that?

We've solved the performance problem due to your insight, so ISTM ok
now to enable AELs on subxids, since not having them causes locks to
be held for too long, which is a problem also. Rearranging the code
doesn't gain us any further performance, but as you say it prevents
prevents us solving the AELs on subxids problem.

Given that, do you agree to me applying assign_aels_against_subxids.v1.patch
as well?

We can discuss any backpatches after the CF is done.

> 0004:
>
> Again master only, is a further small optimization and of
> StandbyReleaseLocks() to further tighten the slow loop over each held
> lock. I also think that it's better to be more explicit about the case
> of when the function would release all locks. Although I'm not all
> that sure in which case the function will actually be called with an
> invalid xid.
>
> Patch series is attached.

This does improve things a little more, but we already hit 95% of the
gain, so I'd prefer to keep the code as similar as possible.

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