On Thu, Jan 11, 2024 at 09:47:33AM -0500, Jonathan S. Katz wrote: > I have similar data sources to Nathan/Michael and I'm trying to avoid piling > on, but one case that's interesting occurred after a major version upgrade > from PG10 to PG14 on a database supporting a very active/highly concurrent > workload. On inspection, it seems like backpatching would help this > particularly case. > > With 10/11 EOL, I do wonder if we'll see more of these reports on upgrade to > < PG16. > > (I was in favor of backpatching prior; opinion is unchanged).
Hearing nothing, I have prepared a set of patches for v12~v15, checking all the lwlock paths for all the branches. At the end the set of changes look rather sane to me regarding the queue handlings. I have also run some numbers on all the branches, and the test case posted upthread falls off dramatically after 512 concurrent connections at the top of all the stable branches :( For example on REL_12_STABLE with and without the patch attached: num v12 v12+patch 1 29717.151665 29096.707588 2 63257.709301 61889.476318 4 127921.873393 124575.901330 8 231400.571662 230562.725174 16 343911.185351 312432.897015 32 291748.985280 281011.787701 64 268998.728648 269975.605115 128 297332.597018 286449.176950 256 243902.817657 240559.122309 512 190069.602270 194510.718508 768 58915.650225 165714.707198 1024 39920.950552 149433.836901 2048 16922.391688 108164.301054 4096 6229.063321 69032.338708 I'd like to apply that, just let me know if you have any comments and/or objections. -- Michael
From 83706cd5792bc002a668fd54cb93c15011a97063 Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 20 Nov 2022 11:56:32 -0800 Subject: [PATCH] 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. As the quadratic behavior arguably is a bug, we might want to decide to backpatch this fix in the future. Author: Andres Freund <and...@anarazel.de> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6...@awork3.anarazel.de Discussion: https://postgr.es/m/CALj2ACXktNbG=k8xi7psqboftzozavhaxjatvc14iyalu4m...@mail.gmail.com --- src/include/storage/lwlock.h | 8 ++++ src/include/storage/proc.h | 2 +- src/backend/access/transam/twophase.c | 2 +- src/backend/storage/lmgr/lwlock.c | 53 +++++++++++++++------------ src/backend/storage/lmgr/proc.c | 4 +- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index e03d317eea..d88fa4b4ad 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -23,6 +23,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 2579e619eb..2ea773098b 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -211,7 +211,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 */ diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 5293c6920b..ca7037eb2f 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -486,7 +486,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 3bb448627c..ea34ee785c 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -987,6 +987,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. @@ -1044,7 +1053,7 @@ LWLockWakeup(LWLock *lock) * another lock. */ pg_write_barrier(); - waiter->lwWaiting = false; + waiter->lwWaiting = LW_WS_NOT_WAITING; PGSemaphoreUnlock(waiter->sem); } } @@ -1065,7 +1074,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); @@ -1073,7 +1082,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 */ @@ -1100,8 +1109,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; @@ -1114,18 +1122,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) @@ -1137,8 +1140,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; @@ -1162,7 +1165,7 @@ LWLockDequeueSelf(LWLock *lock) for (;;) { PGSemaphoreLock(MyProc->sem); - if (!MyProc->lwWaiting) + if (MyProc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1313,7 +1316,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1478,7 +1481,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1694,7 +1697,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++; } @@ -1772,6 +1775,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. */ @@ -1787,7 +1794,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 1b514f44e3..c60707b9db 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -397,7 +397,7 @@ InitProcess(void) /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */ if (IsAutoVacuumWorkerProcess()) MyProc->statusFlags |= PROC_IS_AUTOVACUUM; - MyProc->lwWaiting = false; + MyProc->lwWaiting = LW_WS_NOT_WAITING; MyProc->lwWaitMode = 0; MyProc->waitLock = NULL; MyProc->waitProcLock = NULL; @@ -579,7 +579,7 @@ InitAuxiliaryProcess(void) MyProc->isBackgroundWorker = IsBackgroundWorker; MyProc->delayChkptFlags = 0; MyProc->statusFlags = 0; - MyProc->lwWaiting = false; + MyProc->lwWaiting = LW_WS_NOT_WAITING; MyProc->lwWaitMode = 0; MyProc->waitLock = NULL; MyProc->waitProcLock = NULL; -- 2.43.0
From 6066487bc301e5f725a1f0c2e696ccde0f5e06bf Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 20 Nov 2022 11:56:32 -0800 Subject: [PATCH] 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. As the quadratic behavior arguably is a bug, we might want to decide to backpatch this fix in the future. Author: Andres Freund <and...@anarazel.de> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6...@awork3.anarazel.de Discussion: https://postgr.es/m/CALj2ACXktNbG=k8xi7psqboftzozavhaxjatvc14iyalu4m...@mail.gmail.com --- src/include/storage/lwlock.h | 8 ++++ src/include/storage/proc.h | 2 +- src/backend/access/transam/twophase.c | 2 +- src/backend/storage/lmgr/lwlock.c | 53 +++++++++++++++------------ src/backend/storage/lmgr/proc.c | 4 +- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index 78afe57fb2..b2c9a9dbbc 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -23,6 +23,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 1464fad9b9..9cc37aa0d9 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -180,7 +180,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 */ diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 076315e7a2..db68b14443 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -482,7 +482,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 d63dc1b93e..28a540961e 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -993,6 +993,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. @@ -1050,7 +1059,7 @@ LWLockWakeup(LWLock *lock) * another lock. */ pg_write_barrier(); - waiter->lwWaiting = false; + waiter->lwWaiting = LW_WS_NOT_WAITING; PGSemaphoreUnlock(waiter->sem); } } @@ -1071,7 +1080,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); @@ -1079,7 +1088,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 */ @@ -1107,8 +1116,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; @@ -1121,18 +1129,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) @@ -1144,8 +1147,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; @@ -1169,7 +1172,7 @@ LWLockDequeueSelf(LWLock *lock) for (;;) { PGSemaphoreLock(MyProc->sem); - if (!MyProc->lwWaiting) + if (MyProc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1320,7 +1323,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1485,7 +1488,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1701,7 +1704,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++; } @@ -1779,6 +1782,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. */ @@ -1794,7 +1801,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 1e06d6580c..f62e622a7b 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -400,7 +400,7 @@ InitProcess(void) /* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */ if (IsAutoVacuumWorkerProcess()) MyProc->statusFlags |= 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) MyProc->delayChkpt = false; MyProc->delayChkptEnd = false; MyProc->statusFlags = 0; - MyProc->lwWaiting = false; + MyProc->lwWaiting = LW_WS_NOT_WAITING; MyProc->lwWaitMode = 0; MyProc->waitLock = NULL; MyProc->waitProcLock = NULL; -- 2.43.0
From 2cecd5135e294fbb81ff726ab2f46baf2a73d69c Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 20 Nov 2022 11:56:32 -0800 Subject: [PATCH] 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. As the quadratic behavior arguably is a bug, we might want to decide to backpatch this fix in the future. Author: Andres Freund <and...@anarazel.de> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6...@awork3.anarazel.de Discussion: https://postgr.es/m/CALj2ACXktNbG=k8xi7psqboftzozavhaxjatvc14iyalu4m...@mail.gmail.com --- src/include/storage/lwlock.h | 8 ++++ src/include/storage/proc.h | 2 +- src/backend/access/transam/twophase.c | 2 +- src/backend/storage/lmgr/lwlock.c | 53 +++++++++++++++------------ src/backend/storage/lmgr/proc.c | 4 +- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index cdbfbed118..a1197a8007 100644 --- 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 7c85b5645b..ffb185d3df 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -140,7 +140,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 */ diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index d375b1012b..f8272c2f0d 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -484,7 +484,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 db89137c03..49423ea4d2 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -998,6 +998,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. @@ -1055,7 +1064,7 @@ LWLockWakeup(LWLock *lock) * another lock. */ pg_write_barrier(); - waiter->lwWaiting = false; + waiter->lwWaiting = LW_WS_NOT_WAITING; PGSemaphoreUnlock(waiter->sem); } } @@ -1076,7 +1085,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); @@ -1084,7 +1093,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 */ @@ -1112,8 +1121,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; @@ -1126,18 +1134,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) @@ -1149,8 +1152,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; @@ -1174,7 +1177,7 @@ LWLockDequeueSelf(LWLock *lock) for (;;) { PGSemaphoreLock(MyProc->sem); - if (!MyProc->lwWaiting) + if (MyProc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1325,7 +1328,7 @@ LWLockAcquire(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1490,7 +1493,7 @@ LWLockAcquireOrWait(LWLock *lock, LWLockMode mode) for (;;) { PGSemaphoreLock(proc->sem); - if (!proc->lwWaiting) + if (proc->lwWaiting == LW_WS_NOT_WAITING) break; extraWaits++; } @@ -1706,7 +1709,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++; } @@ -1787,6 +1790,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. */ @@ -1802,7 +1809,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 199c0eae07..81a2025eb5 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -402,7 +402,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; @@ -582,7 +582,7 @@ InitAuxiliaryProcess(void) MyProc->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; -- 2.43.0
From 52b403443ac4a9986af6d95d175606332bbc538e Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Sun, 20 Nov 2022 11:56:32 -0800 Subject: [PATCH] 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. As the quadratic behavior arguably is a bug, we might want to decide to backpatch this fix in the future. Author: Andres Freund <and...@anarazel.de> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> Discussion: https://postgr.es/m/20221027165914.2hofzp4cvutj6...@awork3.anarazel.de Discussion: https://postgr.es/m/CALj2ACXktNbG=k8xi7psqboftzozavhaxjatvc14iyalu4m...@mail.gmail.com Backpatch-through: 12 --- src/include/storage/lwlock.h | 8 ++++ src/include/storage/proc.h | 2 +- src/backend/access/transam/twophase.c | 2 +- src/backend/storage/lmgr/lwlock.c | 53 +++++++++++++++------------ src/backend/storage/lmgr/proc.c | 4 +- 5 files changed, 42 insertions(+), 27 deletions(-) diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index 87b1ce083d..a30a7d08e9 100644 --- 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 7024df206d..5edb22f49d 100644 --- 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 */ diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index e9ab612717..71b54e0292 100644 --- 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 6b6e920704..933831edf5 100644 --- 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 80a8b48c3c..83b0610dce 100644 --- 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; -- 2.43.0
signature.asc
Description: PGP signature
