From 77c6f747011c0ff8771414ded1a23306e6e953c9 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 27 Oct 2022 10:56:39 +1300 Subject: [PATCH v3 6/6] Don't re-acquire LockManager partition lock after wakeup. Change the contract of WaitOnLock() and ProcSleep() to return with the partition lock released. ProcSleep() re-acquired the lock to prevent interrupts, which was a problem at the time the ancestor of that code was committed in fe548629c50, because then we had signal handlers that longjmp'd out of there. Now, that can happen only at points where we explicitly run CHECK_FOR_INTERRUPTS(), so we can remove the lock, which leads to the idea of making the function always release. While an earlier commit fixed the problem that the backend woke us up before it had even released the lock itself, this lock reacquisition point was still contended when multiple backends woke at the same time. XXX Right? Or what am I missing? Discussion: https://postgr.es/m/CA%2BhUKGKmO7ze0Z6WXKdrLxmvYa%3DzVGGXOO30MMktufofVwEm1A%40mail.gmail.com --- src/backend/storage/lmgr/lock.c | 18 ++++++++++++------ src/backend/storage/lmgr/proc.c | 20 ++++++++++++-------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index fec6e52145..97be94dbd3 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -787,6 +787,7 @@ LockAcquireExtended(const LOCKTAG *locktag, LWLock *partitionLock; bool found_conflict; bool log_lock = false; + bool partitionLocked = false; LatchGroup wakeups = {0}; if (lockmethodid <= 0 || lockmethodid >= lengthof(LockMethods)) @@ -995,6 +996,7 @@ LockAcquireExtended(const LOCKTAG *locktag, partitionLock = LockHashPartitionLock(hashcode); LWLockAcquire(partitionLock, LW_EXCLUSIVE); + partitionLocked = true; /* * Find or create lock and proclock entries with this tag @@ -1099,7 +1101,10 @@ LockAcquireExtended(const LOCKTAG *locktag, locktag->locktag_type, lockmode); + Assert(LWLockHeldByMeInMode(partitionLock, LW_EXCLUSIVE)); WaitOnLock(locallock, owner, &wakeups); + Assert(!LWLockHeldByMeInMode(partitionLock, LW_EXCLUSIVE)); + partitionLocked = false; TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field1, locktag->locktag_field2, @@ -1124,7 +1129,6 @@ LockAcquireExtended(const LOCKTAG *locktag, PROCLOCK_PRINT("LockAcquire: INCONSISTENT", proclock); LOCK_PRINT("LockAcquire: INCONSISTENT", lock, lockmode); /* Should we retry ? */ - LWLockRelease(partitionLock); elog(ERROR, "LockAcquire failed"); } PROCLOCK_PRINT("LockAcquire: granted", proclock); @@ -1137,9 +1141,11 @@ LockAcquireExtended(const LOCKTAG *locktag, */ FinishStrongLockAcquire(); - LWLockRelease(partitionLock); - - SetLatches(&wakeups); + if (partitionLocked) + { + LWLockRelease(partitionLock); + SetLatches(&wakeups); + } /* * Emit a WAL record if acquisition of this lock needs to be replayed in a @@ -1806,7 +1812,8 @@ MarkLockClear(LOCALLOCK *locallock) * Caller must have set MyProc->heldLocks to reflect locks already held * on the lockable object by this process. * - * The appropriate partition lock must be held at entry. + * The appropriate partition lock must be held at entry. It is not held on + * exit. */ static void WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, LatchGroup *wakeups) @@ -1851,7 +1858,6 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner, LatchGroup *wakeups) awaitedLock = NULL; LOCK_PRINT("WaitOnLock: aborting on lock", locallock->lock, locallock->tag.mode); - LWLockRelease(LockHashPartitionLock(locallock->hashcode)); /* * Now that we aren't holding the partition lock, we can give an diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 156d293210..35720346f2 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -992,7 +992,7 @@ AuxiliaryPidGetProc(int pid) * Caller must have set MyProc->heldLocks to reflect locks already held * on the lockable object by this process (under all XIDs). * - * The lock table's partition lock must be held at entry, and will be held + * The lock table's partition lock must be held at entry, and will be released * at exit. * * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR if not (deadlock). @@ -1020,6 +1020,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, LatchGroup *wakeups) ProcWaitStatus myWaitStatus; PGPROC *leader = MyProc->lockGroupLeader; + /* + * Every way out of this function will release the partition lock and send + * buffered latch wakeups. + */ + Assert(LWLockHeldByMeInMode(partitionLock, LW_EXCLUSIVE)); + /* * If group locking is in use, locks held by members of my locking group * need to be included in myHeldLocks. This is not required for relation @@ -1104,6 +1110,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, LatchGroup *wakeups) /* Skip the wait and just grant myself the lock. */ GrantLock(lock, proclock, lockmode); GrantAwaitedLock(); + LWLockRelease(partitionLock); + SetLatches(wakeups); return PROC_WAIT_STATUS_OK; } @@ -1140,6 +1148,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, LatchGroup *wakeups) if (early_deadlock) { RemoveFromWaitQueue(MyProc, hashcode, wakeups); + LWLockRelease(partitionLock); + SetLatches(wakeups); return PROC_WAIT_STATUS_ERROR; } @@ -1469,6 +1479,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, LatchGroup *wakeups) } LWLockRelease(partitionLock); + SetLatches(wakeups); if (deadlock_state == DS_SOFT_DEADLOCK) ereport(LOG, @@ -1570,13 +1581,6 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable, LatchGroup *wakeups) standbyWaitStart, GetCurrentTimestamp(), NULL, false); - /* - * Re-acquire the lock table's partition lock. We have to do this to hold - * off cancel/die interrupts before we can mess with lockAwaited (else we - * might have a missed or duplicated locallock update). - */ - LWLockAcquire(partitionLock, LW_EXCLUSIVE); - /* * We no longer want LockErrorCleanup to do anything. */ -- 2.39.2