From 959f569af8da9a216de0e85c008e1d372fcd1c50 Mon Sep 17 00:00:00 2001 From: NikolayS Date: Sat, 25 Jul 2026 07:28:56 +0000 Subject: [PATCH] PROTOTYPE: move NOTIFY queue insertion past commit (not for upstream) Measurement prototype for the LISTEN/NOTIFY commit-lock work in #82. Not a patch proposal -- see "Known gaps" below. PreCommit_Notify() takes a cluster-wide heavyweight lock and inserts the transaction's notifications while holding it; the lock is not released until end of transaction. Since that is after RecordTransactionCommit(), the serialized region contains XLogFlush() and SyncRepWaitForLSN(), so notifying transactions cannot group-commit and throughput is pinned near one commit round trip at a time regardless of concurrency. Move the insertion into PostCommitInsert_Notify(), called from CommitTransaction() immediately after ProcArrayEndTransaction(), and serialize it with a dedicated NotifyQueueInsertLock instead. Placing it after the ProcArray removal rather than before is what makes this work. The listener's gate is XidInMVCCSnapshot(), so the invariant that matters is insertion order == snapshot visibility order, not commit-LSN order. Every entry is now inserted by a transaction that has already left the ProcArray, and a listener reads the queue head before taking its snapshot, so it can never observe an entry whose xid is still in progress. The invariant holds trivially rather than being derived from how long a lock is held. A backend queued on the insert lock also no longer pins its xid/xmin, so it cannot hold back the global horizon or delay the visibility of an already-durable commit. Using an LWLock rather than the heavyweight lock is a correctness requirement, not a tidiness one. Past the commit record, any ERROR reaches RecordTransactionAbort()'s "cannot abort transaction %u, it was already committed" PANIC. LockSharedObject() can throw -- out of shared memory for the lock table, OOM, deadlock detection, and AcceptInvalidationMessages() running catalog callbacks. LWLockAcquire() allocates nothing, consults no lock table, runs no deadlock detector, and is released by LWLockReleaseAll() during error recovery. The old placement was also inside HOLD_INTERRUPTS(), which made the wait uncancellable and unkillable; that is gone too. Measured on 4 vCPU, fsync=on, zero listeners, 3 reps, median of "BEGIN; INSERT; NOTIFY; END" versus the same transaction without NOTIFY: clients control stock this patch 4 2,059 1,033 2,129 16 7,811 1,027 8,155 64 16,307 1,055 16,507 15.7x over stock at 64 clients, with commit latency falling from 60.7ms to 3.9ms -- indistinguishable from the NOTIFY-free control. Known gaps that must be closed before this is a real patch: - asyncQueueAddEntries() can still reach SlruReportIOError() (ENOSPC on pg_notify/, read or close failures), which would PANIC post-commit. Needs either pre-faulting of the target pages during pre-commit, or a decision to degrade to WARNING and drop, which async.c's existing "notifications are not expected to survive database crashes" contract would justify. That is a semantics call for a committer. - asyncQueueIsFull() still ereport(ERROR)s from the post-commit path. Needs capacity (not positional) reservation during pre-commit. - No test would catch an ordering regression. The async regression test and all 130 isolation tests pass, but they do not appear to exercise two concurrent notifiers racing a listener's snapshot. An injection-point test is required. - Not benchmarked with listeners attached, with a synchronous standby, or on hardware where the numbers would be publishable. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW --- src/backend/access/transam/xact.c | 20 +++ src/backend/commands/async.c | 145 ++++++++++-------- .../storage/lmgr/lwlocknames.h.tmp23721 | 22 +++ .../storage/lmgr/lwlocknames.h.tmp23748 | 22 +++ .../utils/activity/wait_event_names.txt | 1 + src/include/commands/async.h | 1 + src/include/storage/lwlocklist.h | 2 +- 7 files changed, 147 insertions(+), 66 deletions(-) create mode 100644 src/backend/storage/lmgr/lwlocknames.h.tmp23721 create mode 100644 src/backend/storage/lmgr/lwlocknames.h.tmp23748 diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 3a89149016fe6..b63be24de4794 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2430,6 +2430,26 @@ CommitTransaction(void) */ ProcArrayEndTransaction(MyProc, latestXid); + /* + * PROTOTYPE ("Alt B"): insert this transaction's notifications into the + * queue now. This is deliberately *after* ProcArrayEndTransaction(): + * + * - The serializing lock is acquired after XLogFlush()/SyncRepWaitForLSN(), + * so notifying transactions no longer serialize across their own commit + * round trip and can group-commit with one another. + * + * - We have already left the ProcArray, so a backend queued on the insert + * lock no longer pins its xid/xmin, and so does not hold back the global + * horizon or delay the visibility of an already-durable commit. + * + * - Every entry is therefore inserted by a transaction that is already + * ProcArray-complete. A listener reads the queue head before taking its + * snapshot, so it can never observe an entry whose xid is still + * in-progress: insertion-order == visibility-order holds trivially + * rather than being derived from how long the lock is held. + */ + PostCommitInsert_Notify(); + /* * This is all post-commit cleanup. Note that if an error is raised here, * it's too late to abort the transaction. This should be just diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index 2799f989b96dc..53340593330f7 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -1226,9 +1226,6 @@ PreCommit_Notify(void) /* Queue any pending notifies (must happen after the above) */ if (pendingNotifies) { - ListCell *nextNotify; - bool firstIteration = true; - /* * Build list of unique channel names being notified for use by * SignalBackends(). @@ -1291,75 +1288,93 @@ PreCommit_Notify(void) (void) GetCurrentTransactionId(); /* - * Serialize writers by acquiring a special lock that we hold till - * after commit. This ensures that queue entries appear in commit - * order, and in particular that there are never uncommitted queue - * entries ahead of committed ones, so an uncommitted transaction - * can't block delivery of deliverable notifications. - * - * We use a heavyweight lock so that it'll automatically be released - * after either commit or abort. This also allows deadlocks to be - * detected, though really a deadlock shouldn't be possible here. - * - * The lock is on "database 0", which is pretty ugly but it doesn't - * seem worth inventing a special locktag category just for this. - * (Historical note: before PG 9.0, a similar lock on "database 0" was - * used by the flatfiles mechanism.) + * PROTOTYPE ("Alt B"): the queue insertion, and the serializing lock + * that used to be taken here, have moved to PostCommitInsert_Notify(), + * which CommitTransaction() calls after RecordTransactionCommit(). + * Nothing further to do at pre-commit time. */ - LockSharedObject(DatabaseRelationId, InvalidOid, 0, - AccessExclusiveLock); + } +} - /* - * For the direct advancement optimization in SignalBackends(), we - * need to ensure that no other backend can insert queue entries - * between queueHeadBeforeWrite and queueHeadAfterWrite. The - * heavyweight lock above provides this guarantee, since it serializes - * all writers. - * - * Note: if the heavyweight lock were ever removed for scalability - * reasons, we could achieve the same guarantee by holding - * NotifyQueueLock in EXCLUSIVE mode across all our insertions, rather - * than releasing and reacquiring it for each page as we do below. - */ +/* + * PostCommitInsert_Notify + * + * PROTOTYPE -- NOT A PATCH PROPOSAL. See notify-bench/README.md. + * + * Insert this transaction's notifications into the queue, after its + * commit record is durable but before it leaves the ProcArray. + * + * The point of moving the insertion to here is that the serializing lock + * is then acquired *after* XLogFlush() and SyncRepWaitForLSN(), so + * notifying transactions no longer serialize across their own commit + * round trip and can group-commit with one another again. + * + * The lock is still held from here until end of transaction, so it still + * spans ProcArrayEndTransaction(). That is deliberate and load-bearing: + * the listener's gate is XidInMVCCSnapshot() (see + * asyncQueueProcessPageEntries), so the invariant that must be preserved + * is *queue insertion order == snapshot visibility order*, not commit-LSN + * order. Holding across the ProcArray removal preserves it exactly, and + * also preserves the precondition that SignalBackends()'s direct-advance + * optimization depends on. + * + * KNOWN DEFECT: asyncQueueIsFull() can still ereport(ERROR) here, and we + * are now past the commit point, so that error would be raised for an + * already-committed transaction. A real patch must reserve quota during + * pre-commit (where failing is safe) and make this path infallible. The + * benchmark never fills the queue, so this does not affect the numbers. + */ +void +PostCommitInsert_Notify(void) +{ + ListCell *nextNotify; + bool firstIteration = true; - /* Initialize values to a safe default in case list is empty */ - SET_QUEUE_POS(queueHeadBeforeWrite, 0, 0); - SET_QUEUE_POS(queueHeadAfterWrite, 0, 0); + if (!pendingNotifies) + return; - /* Now push the notifications into the queue */ - nextNotify = list_head(pendingNotifies->events); - while (nextNotify != NULL) + /* + * Serialize insertions with a dedicated LWLock rather than the old + * heavyweight lock. At this point we are past the commit record, so any + * ereport(ERROR) would reach RecordTransactionAbort()'s "cannot abort + * transaction %u, it was already committed" PANIC. LWLockAcquire() + * allocates nothing, consults no lock table, runs no deadlock detector and + * does not call AcceptInvalidationMessages(), so unlike LockSharedObject() + * it cannot throw. It is also released by LWLockReleaseAll() during error + * recovery, and is not subject to the uninterruptible-wait problem that a + * heavyweight lock wait has here (we are inside HOLD_INTERRUPTS()). + * + * Lock ordering: NotifyQueueInsertLock is taken before NotifyQueueLock, + * which is taken before the SLRU bank locks. + */ + LWLockAcquire(NotifyQueueInsertLock, LW_EXCLUSIVE); + + /* Initialize values to a safe default in case list is empty */ + SET_QUEUE_POS(queueHeadBeforeWrite, 0, 0); + SET_QUEUE_POS(queueHeadAfterWrite, 0, 0); + + nextNotify = list_head(pendingNotifies->events); + while (nextNotify != NULL) + { + LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE); + if (firstIteration) { - /* - * Add the pending notifications to the queue. We acquire and - * release NotifyQueueLock once per page, which might be overkill - * but it does allow readers to get in while we're doing this. - * - * A full queue is very uncommon and should really not happen, - * given that we have so much space available in the SLRU pages. - * Nevertheless we need to deal with this possibility. Note that - * when we get here we are in the process of committing our - * transaction, but we have not yet committed to clog, so at this - * point in time we can still roll the transaction back. - */ - LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE); - if (firstIteration) - { - queueHeadBeforeWrite = QUEUE_HEAD; - firstIteration = false; - } - asyncQueueFillWarning(); - if (asyncQueueIsFull()) - ereport(ERROR, - (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), - errmsg("too many notifications in the NOTIFY queue"))); - nextNotify = asyncQueueAddEntries(nextNotify); - queueHeadAfterWrite = QUEUE_HEAD; - LWLockRelease(NotifyQueueLock); + queueHeadBeforeWrite = QUEUE_HEAD; + firstIteration = false; } - - /* Note that we don't clear pendingNotifies; AtCommit_Notify will. */ + asyncQueueFillWarning(); + if (asyncQueueIsFull()) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("too many notifications in the NOTIFY queue"))); + nextNotify = asyncQueueAddEntries(nextNotify); + queueHeadAfterWrite = QUEUE_HEAD; + LWLockRelease(NotifyQueueLock); } + + LWLockRelease(NotifyQueueInsertLock); + + /* Note that we don't clear pendingNotifies; AtCommit_Notify will. */ } /* diff --git a/src/backend/storage/lmgr/lwlocknames.h.tmp23721 b/src/backend/storage/lmgr/lwlocknames.h.tmp23721 new file mode 100644 index 0000000000000..ff1dfafffef76 --- /dev/null +++ b/src/backend/storage/lmgr/lwlocknames.h.tmp23721 @@ -0,0 +1,22 @@ +/* autogenerated from src/include/storage/lwlocklist.h, do not edit */ +/* there is deliberately not an #ifndef LWLOCKNAMES_H here */ + +#define OidGenLock (&MainLWLockArray[2].lock) +#define XidGenLock (&MainLWLockArray[3].lock) +#define ProcArrayLock (&MainLWLockArray[4].lock) +#define SInvalReadLock (&MainLWLockArray[5].lock) +#define SInvalWriteLock (&MainLWLockArray[6].lock) +#define WALBufMappingLock (&MainLWLockArray[7].lock) +#define WALWriteLock (&MainLWLockArray[8].lock) +#define ControlFileLock (&MainLWLockArray[9].lock) +#define MultiXactGenLock (&MainLWLockArray[13].lock) +#define RelCacheInitLock (&MainLWLockArray[16].lock) +#define CheckpointerCommLock (&MainLWLockArray[17].lock) +#define TwoPhaseStateLock (&MainLWLockArray[18].lock) +#define TablespaceCreateLock (&MainLWLockArray[19].lock) +#define BtreeVacuumLock (&MainLWLockArray[20].lock) +#define AddinShmemInitLock (&MainLWLockArray[21].lock) +#define AutovacuumLock (&MainLWLockArray[22].lock) +#define AutovacuumScheduleLock (&MainLWLockArray[23].lock) +#define SyncScanLock (&MainLWLockArray[24].lock) +#define RelationMappingLock (&MainLWLockArray[25].lock) diff --git a/src/backend/storage/lmgr/lwlocknames.h.tmp23748 b/src/backend/storage/lmgr/lwlocknames.h.tmp23748 new file mode 100644 index 0000000000000..ff1dfafffef76 --- /dev/null +++ b/src/backend/storage/lmgr/lwlocknames.h.tmp23748 @@ -0,0 +1,22 @@ +/* autogenerated from src/include/storage/lwlocklist.h, do not edit */ +/* there is deliberately not an #ifndef LWLOCKNAMES_H here */ + +#define OidGenLock (&MainLWLockArray[2].lock) +#define XidGenLock (&MainLWLockArray[3].lock) +#define ProcArrayLock (&MainLWLockArray[4].lock) +#define SInvalReadLock (&MainLWLockArray[5].lock) +#define SInvalWriteLock (&MainLWLockArray[6].lock) +#define WALBufMappingLock (&MainLWLockArray[7].lock) +#define WALWriteLock (&MainLWLockArray[8].lock) +#define ControlFileLock (&MainLWLockArray[9].lock) +#define MultiXactGenLock (&MainLWLockArray[13].lock) +#define RelCacheInitLock (&MainLWLockArray[16].lock) +#define CheckpointerCommLock (&MainLWLockArray[17].lock) +#define TwoPhaseStateLock (&MainLWLockArray[18].lock) +#define TablespaceCreateLock (&MainLWLockArray[19].lock) +#define BtreeVacuumLock (&MainLWLockArray[20].lock) +#define AddinShmemInitLock (&MainLWLockArray[21].lock) +#define AutovacuumLock (&MainLWLockArray[22].lock) +#define AutovacuumScheduleLock (&MainLWLockArray[23].lock) +#define SyncScanLock (&MainLWLockArray[24].lock) +#define RelationMappingLock (&MainLWLockArray[25].lock) diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt index 1016502d042eb..03c0b2424412c 100644 --- a/src/backend/utils/activity/wait_event_names.txt +++ b/src/backend/utils/activity/wait_event_names.txt @@ -343,6 +343,7 @@ Autovacuum "Waiting to read or update the current state of autovacuum workers." AutovacuumSchedule "Waiting to ensure that a table selected for autovacuum still needs vacuuming." SyncScan "Waiting to select the starting location of a synchronized table scan." RelationMapping "Waiting to read or update a pg_filenode.map file (used to track the filenode assignments of certain system catalogs)." +NotifyQueueInsert "Waiting to insert NOTIFY messages into the queue at commit." NotifyQueue "Waiting to read or update NOTIFY messages." SerializableXactHash "Waiting to read or update information about serializable transactions." SerializableFinishedList "Waiting to access the list of finished serializable transactions." diff --git a/src/include/commands/async.h b/src/include/commands/async.h index 202e4aa5e7452..cdd768c30f077 100644 --- a/src/include/commands/async.h +++ b/src/include/commands/async.h @@ -31,6 +31,7 @@ extern void Async_UnlistenAll(void); /* perform (or cancel) outbound notify processing at transaction commit */ extern void PreCommit_Notify(void); +extern void PostCommitInsert_Notify(void); extern void AtCommit_Notify(void); extern void AtAbort_Notify(void); extern void AtSubCommit_Notify(void); diff --git a/src/include/storage/lwlocklist.h b/src/include/storage/lwlocklist.h index d7eb648bd2758..4dadda0a770c5 100644 --- a/src/include/storage/lwlocklist.h +++ b/src/include/storage/lwlocklist.h @@ -57,7 +57,7 @@ PG_LWLOCK(22, Autovacuum) PG_LWLOCK(23, AutovacuumSchedule) PG_LWLOCK(24, SyncScan) PG_LWLOCK(25, RelationMapping) -/* 26 was NotifySLRULock */ +PG_LWLOCK(26, NotifyQueueInsert) PG_LWLOCK(27, NotifyQueue) PG_LWLOCK(28, SerializableXactHash) PG_LWLOCK(29, SerializableFinishedList)