lwlock: Fix quadratic behavior with very long wait lists

Until now LWLockDequeueSelf() sequentially searched the list of waiters to see
if the current proc is still is on the list of waiters, or has already been
removed. In extreme workloads, where the wait lists are very long, this leads
to a quadratic behavior. #backends iterating over a list #backends
long. Additionally, the likelihood of needing to call LWLockDequeueSelf() in
the first place also increases with the increased length of the wait queue, as
it becomes more likely that a lock is released while waiting for the wait list
lock, which is held for longer during lock release.

Due to the exponential back-off in perform_spin_delay() this is surprisingly
hard to detect. We should make that easier, e.g. by adding a wait event around
the pg_usleep() - but that's a separate patch.

The fix is simple - track whether a proc is currently waiting in the wait list
or already removed but waiting to be woken up in PGPROC->lwWaiting.

In some workloads with a lot of clients contending for a small number of
lwlocks (e.g. WALWriteLock), the fix can substantially increase throughput.

This has been originally fixed for 16~ with a4adc31f6902 without a
backpatch, and we have heard complaints from users impacted by this
quadratic behavior in older versions as well.

Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6gin@awork3.anarazel.de
Discussion: https://postgr.es/m/CALj2ACXktNbG=K8Xi7PSqbofTZozavhaxjatVc14iYaLu4Maag@mail.gmail.com
Backpatch-through: 12

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index e9ab612717ebda091375eff26a28c881b94a2711..71b54e0292d163cdd8e605c60d79ba04156f0355 100644 (file)
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -485,7 +485,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
        proc->roleId = owner;
        proc->tempNamespaceId = InvalidOid;
        proc->isBackgroundWorker = false;
-       proc->lwWaiting = false;
+       proc->lwWaiting = LW_WS_NOT_WAITING;
        proc->lwWaitMode = 0;
        proc->waitLock = NULL;
        proc->waitProcLock = NULL;

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 6b6e92070411fd5e2719e8c6b751da3155e0d803..933831edf5708d77885f25f14e1eaba5aa92c28a 100644 (file)
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -913,6 +913,15 @@ LWLockWakeup(LWLock *lock)
                        wokeup_somebody = true;
                }
 
+               /*
+                * Signal that the process isn't on the wait list anymore. This allows
+                * LWLockDequeueSelf() to remove itself of the waitlist with a
+                * proclist_delete(), rather than having to check if it has been
+                * removed from the list.
+                */
+               Assert(waiter->lwWaiting == LW_WS_WAITING);
+               waiter->lwWaiting = LW_WS_PENDING_WAKEUP;
+
                /*
                 * Once we've woken up an exclusive lock, there's no point in waking
                 * up anybody else.
@@ -970,7 +979,7 @@ LWLockWakeup(LWLock *lock)
                 * another lock.
                 */
                pg_write_barrier();
-               waiter->lwWaiting = false;
+               waiter->lwWaiting = LW_WS_NOT_WAITING;
                PGSemaphoreUnlock(waiter->sem);
        }
 }
@@ -991,7 +1000,7 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
        if (MyProc == NULL)
                elog(PANIC, "cannot wait without a PGPROC structure");
 
-       if (MyProc->lwWaiting)
+       if (MyProc->lwWaiting != LW_WS_NOT_WAITING)
                elog(PANIC, "queueing for lock while waiting on another one");
 
        LWLockWaitListLock(lock);
@@ -999,7 +1008,7 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
        /* setting the flag is protected by the spinlock */
        pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_HAS_WAITERS);
 
-       MyProc->lwWaiting = true;
+       MyProc->lwWaiting = LW_WS_WAITING;
        MyProc->lwWaitMode = mode;
 
        /* LW_WAIT_UNTIL_FREE waiters are always at the front of the queue */
@@ -1027,8 +1036,7 @@ LWLockQueueSelf(LWLock *lock, LWLockMode mode)
 static void
 LWLockDequeueSelf(LWLock *lock)
 {
-       bool            found = false;
-       proclist_mutable_iter iter;
+       bool            on_waitlist;
 
 #ifdef LWLOCK_STATS
        lwlock_stats *lwstats;
@@ -1041,18 +1049,13 @@ LWLockDequeueSelf(LWLock *lock)
        LWLockWaitListLock(lock);
 
        /*
-        * Can't just remove ourselves from the list, but we need to iterate over
-        * all entries as somebody else could have dequeued us.
+        * Remove ourselves from the waitlist, unless we've already been
+        * removed. The removal happens with the wait list lock held, so there's
+        * no race in this check.
         */
-       proclist_foreach_modify(iter, &lock->waiters, lwWaitLink)
-       {
-               if (iter.cur == MyProc->pgprocno)
-               {
-                       found = true;
-                       proclist_delete(&lock->waiters, iter.cur, lwWaitLink);
-                       break;
-               }
-       }
+       on_waitlist = MyProc->lwWaiting == LW_WS_WAITING;
+       if (on_waitlist)
+               proclist_delete(&lock->waiters, MyProc->pgprocno, lwWaitLink);
 
        if (proclist_is_empty(&lock->waiters) &&
                (pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS) != 0)
@@ -1064,8 +1067,8 @@ LWLockDequeueSelf(LWLock *lock)
        LWLockWaitListUnlock(lock);
 
        /* clear waiting state again, nice for debugging */
-       if (found)
-               MyProc->lwWaiting = false;
+       if (on_waitlist)
+               MyProc->lwWaiting = LW_WS_NOT_WAITING;
        else
        {
                int                     extraWaits = 0;
@@ -1089,7 +1092,7 @@ LWLockDequeueSelf(LWLock *lock)
                for (;;)
                {
                        PGSemaphoreLock(MyProc->sem);
-                       if (!MyProc->lwWaiting)
+                       if (MyProc->lwWaiting == LW_WS_NOT_WAITING)
                                break;
                        extraWaits++;
                }
@@ -1239,7 +1242,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode)
                for (;;)
                {
                        PGSemaphoreLock(proc->sem);
-                       if (!proc->lwWaiting)
+                       if (proc->lwWaiting == LW_WS_NOT_WAITING)
                                break;
                        extraWaits++;
                }
@@ -1399,7 +1402,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode)
                        for (;;)
                        {
                                PGSemaphoreLock(proc->sem);
-                               if (!proc->lwWaiting)
+                               if (proc->lwWaiting == LW_WS_NOT_WAITING)
                                        break;
                                extraWaits++;
                        }
@@ -1611,7 +1614,7 @@ LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval)
                for (;;)
                {
                        PGSemaphoreLock(proc->sem);
-                       if (!proc->lwWaiting)
+                       if (proc->lwWaiting == LW_WS_NOT_WAITING)
                                break;
                        extraWaits++;
                }
@@ -1690,6 +1693,10 @@ LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
 
                proclist_delete(&lock->waiters, iter.cur, lwWaitLink);
                proclist_push_tail(&wakeup, iter.cur, lwWaitLink);
+
+               /* see LWLockWakeup() */
+               Assert(waiter->lwWaiting == LW_WS_WAITING);
+               waiter->lwWaiting = LW_WS_PENDING_WAKEUP;
        }
 
        /* We are done updating shared state of the lock itself. */
@@ -1705,7 +1712,7 @@ LWLockUpdateVar(LWLock *lock, uint64 *valptr, uint64 val)
                proclist_delete(&wakeup, iter.cur, lwWaitLink);
                /* check comment in LWLockWakeup() about this barrier */
                pg_write_barrier();
-               waiter->lwWaiting = false;
+               waiter->lwWaiting = LW_WS_NOT_WAITING;
                PGSemaphoreUnlock(waiter->sem);
        }
 }

diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 80a8b48c3cd9331739a6969c4116d460beba2831..83b0610dcebebf4a696c1af884cb4910a52e08a2 100644 (file)
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -403,7 +403,7 @@ InitProcess(void)
        /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
        if (IsAutoVacuumWorkerProcess())
                MyPgXact->vacuumFlags |= PROC_IS_AUTOVACUUM;
-       MyProc->lwWaiting = false;
+       MyProc->lwWaiting = LW_WS_NOT_WAITING;
        MyProc->lwWaitMode = 0;
        MyProc->waitLock = NULL;
        MyProc->waitProcLock = NULL;
@@ -583,7 +583,7 @@ InitAuxiliaryProcess(void)
        MyPgXact->delayChkpt = false;
        MyProc->delayChkptEnd = false;
        MyPgXact->vacuumFlags = 0;
-       MyProc->lwWaiting = false;
+       MyProc->lwWaiting = LW_WS_NOT_WAITING;
        MyProc->lwWaitMode = 0;
        MyProc->waitLock = NULL;
        MyProc->waitProcLock = NULL;

diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h
index 87b1ce083df174ddf7f685d68a0992cb7337d1de..a30a7d08e9f49af92a9556850dbe1f5a7c39450f 100644 (file)
--- a/src/include/storage/lwlock.h
+++ b/src/include/storage/lwlock.h
@@ -24,6 +24,14 @@
 
 struct PGPROC;
 
+/* what state of the wait process is a backend in */
+typedef enum LWLockWaitState
+{
+       LW_WS_NOT_WAITING, /* not currently waiting / woken up */
+       LW_WS_WAITING, /* currently waiting */
+       LW_WS_PENDING_WAKEUP /* removed from waitlist, but not yet signalled */
+} LWLockWaitState;
+
 /*
  * Code outside of lwlock.c should not manipulate the contents of this
  * structure directly, but we have to declare it here to allow LWLocks to be

diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 7024df206db8a3fc451d46b2eddf29cf3cca3fdb..5edb22f49d3e88087bfa8bbea46c21dc27aeab26 100644 (file)
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -134,7 +134,7 @@ struct PGPROC
        bool            recoveryConflictPending;
 
        /* Info about LWLock the process is currently waiting for, if any. */
-       bool            lwWaiting;              /* true if waiting for an LW lock */
+       uint8           lwWaiting;              /* see LWLockWaitState */
        uint8           lwWaitMode;             /* lwlock mode being waited for */
        proclist_node lwWaitLink;       /* position in LW lock wait list */