Re: Excessive CPU usage in StandbyReleaseLocks()

Hi,

On 2018-06-19 17:43:42 +1200, Thomas Munro wrote:
> The problem is that StandbyReleaseLocks() does a linear search of all
> known AccessExclusiveLocks when a transaction ends.  Luckily, since
> v10 (commit 9b013dc2) that is skipped for transactions that haven't
> taken any AELs and aren't using 2PC, but that doesn't help all users.

> It's fine if the AEL list is short, but if you do something that takes
> a lot of AELs such as restoring a database with many tables or
> truncating a lot of partitions while other transactions are in flight
> then we start doing O(txrate * nlocks * nsubxacts) work and that can
> hurt.
> 
> The reproducer script I've attached creates one long-lived transaction
> that acquires 6,000 AELs and takes a nap, while 48 connections run
> trivial 2PC transactions (I was also able to reproduce the effect
> without 2PC by creating a throw-away temporary table in every
> transaction, but it was unreliable due to contention slowing
> everything down).  For me, the standby's startup process becomes 100%
> pegged, replay_lag begins to climb and perf says something like:
> 
> +   97.88%    96.96%  postgres  postgres            [.] StandbyReleaseLocks
> 
> The attached patch splits the AEL list into one list per xid and
> sticks them in a hash table.  That makes perf say something like:

> +    0.60%     0.00%  postgres  postgres            [.] StandbyReleaseLocks

Neato. Results aside, that seems like a better suited datastructure.

>  /*
>   * InitRecoveryTransactionEnvironment
> @@ -63,6 +73,19 @@ void
>  InitRecoveryTransactionEnvironment(void)
>  {
>      VirtualTransactionId vxid;
> +    HASHCTL            hash_ctl;
> +
> +    /*
> +     * Initialize the hash table for tracking the list of locks held by each
> +     * transaction.
> +     */
> +    memset(&hash_ctl, 0, sizeof(hash_ctl));
> +    hash_ctl.keysize = sizeof(TransactionId);
> +    hash_ctl.entrysize = sizeof(RecoveryLockListsEntry);
> +    RecoveryLockLists = hash_create("RecoveryLockLists",
> +                                    4096,
> +                                    &hash_ctl,

Isn't that initial size needlessly big?


>  void
>  StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
>  {
> +    RecoveryLockListsEntry *entry;
>      xl_standby_lock *newlock;
>      LOCKTAG        locktag;
> +    bool        found;
>  
>      /* Already processed? */
>      if (!TransactionIdIsValid(xid) ||
> @@ -616,11 +641,19 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
>      /* dbOid is InvalidOid when we are locking a shared relation. */
>      Assert(OidIsValid(relOid));
>  
> +    /* Create a new list for this xid, if we don't have one already. */
> +    entry = hash_search(RecoveryLockLists, &xid, HASH_ENTER, &found);
> +    if (!found)
> +    {
> +        entry->xid = xid;
> +        entry->locks = NIL;
> +    }
> +
>      newlock = palloc(sizeof(xl_standby_lock));
>      newlock->xid = xid;
>      newlock->dbOid = dbOid;
>      newlock->relOid = relOid;
> -    RecoveryLockList = lappend(RecoveryLockList, newlock);
> +    entry->locks = lappend(entry->locks, newlock);

Gotta be careful with lappend et al - it's really easy to leak memory
without explicit context usage. Have you checked that we don't here?

>      SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid);
>  
> @@ -628,46 +661,45 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
>  }
>  
>  static void
> -StandbyReleaseLocks(TransactionId xid)
> +StandbyReleaseLockList(List *locks)
>  {
> -    ListCell   *cell,
> -               *prev,
> -               *next;
> -
> -    /*
> -     * Release all matching locks and remove them from list
> -     */
> -    prev = NULL;
> -    for (cell = list_head(RecoveryLockList); cell; cell = next)
> +    while (locks)
>      {
> -        xl_standby_lock *lock = (xl_standby_lock *) lfirst(cell);
> +        xl_standby_lock *lock = (xl_standby_lock *) linitial(locks);
> +        LOCKTAG        locktag;
> +        elog(trace_recovery(DEBUG4),
> +             "releasing recovery lock: xid %u db %u rel %u",
> +             lock->xid, lock->dbOid, lock->relOid);
> +        SET_LOCKTAG_RELATION(locktag, lock->dbOid, lock->relOid);
> +        if (!LockRelease(&locktag, AccessExclusiveLock, true))
> +            elog(LOG,
> +                 "RecoveryLockLists contains entry for lock no longer recorded by lock manager: xid %u database %u
relation%u",
 
> +                 lock->xid, lock->dbOid, lock->relOid);

This should be a PANIC imo.


Greetings,

Andres Freund