Re: Patch to improve performance of replay of AccessExclusiveLock

On 7 March 2017 at 23:20, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 7 March 2017 at 10:01, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>> On 2 March 2017 at 16:06, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Wed, Mar 1, 2017 at 5:32 PM, David Rowley
>>> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>>>> Hackers,
>>>>
>>>> I've attached a small patch which aims to improve the performance of
>>>> AccessExclusiveLock when there are many AccessExclusiveLock stored.
>
> Looks useful.

Thanks.

>> That may need tweaking. Likely it could be smaller if we had some sort
>> of bloom filter to mark if the transaction had obtained any AEL locks,
>> that way it could skip. Initially I really didn't want to make the
>> patch too complex. I had thought that a fairly large hash table would
>> fix the problem well enough, as quite possibly most buckets would be
>> empty and non empty buckets have short lists.
>
> ISTM that we should mark each COMMIT if it has an AEL, so we can avoid
> any overhead in the common case.
>
> So an additional sub-record for the commit/abort wal record, via
> include/access/xact.h

That would be ideal if we could do that, but doing that for so many
possible transaction IDs seems impractical. I had previously thought I
could do this in a lossy way with bloom filters for the hash table,
but I'd failed to realise at that time that I'd be unable to unmark
the bloom filter bit when releasing the locks, as that bit may be used
by other xid.

>>> 4.
>>> In some cases, it might slow down the performance where you have to
>>> traverse the complete hash table like StandbyReleaseOldLocks, however,
>>> I think those cases will be less relative to other lock release calls
>>> which are called at every commit, so probably this is okay.
>>
>> Yes, that may be true. Perhaps making the table smaller would help
>> bring that back to what it was, or perhaps the patch needs rethought
>> to use bloom filters to help identify if we need to release any locks.
>>
>> Opinions on that would be welcome. Meanwhile I'll go off and play with
>> bloom filters to see if I can knock some percentage points of the perf
>> report for StandbyReleaseLocks().
>
> Didn't look at the code closely, but if the common case COMMITs don't
> scan the list that would be most of the problem solved. If we did need
> a hash table, we should just use a normal hash table?

I've tried to address that to some degree and added code which skips
the lookups when there's no AEL locks held at all. There's a
possibility for speeding up replay of COMMIT of a transaction with
many sub transactions where no AEL locks are held at all. See
StandbyReleaseLockTree().

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