Re: Patch to improve performance of replay of AccessExclusiveLock

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

I could see that your idea is quite straightforward to improve
performance (basically, convert recovery lock list to recovery lock
hash table based on xid), however, it is not clear under what kind of
workload this will be helpful? Do you expect that many concurrent
sessions are trying to acquire different AEL locks?

Few comments on the patch:
1.
+/*
+ * Number of buckets to split RecoveryLockTable into.
+ * This must be a power of two.
+ */

+#define RECOVERYLOCKTABLE_SIZE 1024

On what basis did you decide to keep the lock table size as 1024, is
it just a guess, if so, then also adding a comment to indicate that
you think this is sufficient for current use cases seems better.
However, if it is based on some data, then it would be more
appropriate.

2.
@@ -607,6 +627,8 @@ StandbyAcquireAccessExclusiveLock(TransactionId
xid, Oid dbOid, Oid relOid)
{
xl_standby_lock *newlock;
LOCKTAG locktag;
+ size_t pidx;
+

Comment on top of this function needs some change, it still seems to
refer to RelationLockList.

* We keep a single dynamically expandible list of locks in local memory,
* RelationLockList, so we can keep track of the various entries made by
* the Startup process's virtual xid in the shared lock table.

3.
+ size_t pidx;
+
+ if (!TransactionIdIsValid(xid))
+ {
+ StandbyReleaseAllLocks();
+ return;
+ }

What is the need of this new code?

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.

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