PROTOTYPE: move NOTIFY queue insertion past commit (not for upstream)#85
Draft
NikolayS wants to merge 1 commit into
Draft
PROTOTYPE: move NOTIFY queue insertion past commit (not for upstream)#85NikolayS wants to merge 1 commit into
NikolayS wants to merge 1 commit into
Conversation
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 <[email protected]> Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Measurement prototype for the LISTEN/NOTIFY commit-lock work in #82. Not a patch proposal — the known gaps below are disqualifying, and at least one of them is a semantics decision only a committer can make.
Companion PRs: #83 (docs, upstream-bound), #84 (benchmark harness, fork-local).
The change
PreCommit_Notify()takes a cluster-wide heavyweight lock and inserts notifications while holding it, and the lock isn't released until end of transaction — which is afterRecordTransactionCommit(). So the serialized region containsXLogFlush()andSyncRepWaitForLSN(), notifying transactions cannot group-commit, and throughput is pinned near one commit round trip at a time regardless of concurrency.This moves the insertion to
PostCommitInsert_Notify(), called fromCommitTransaction()immediately afterProcArrayEndTransaction(), serialized by a dedicatedNotifyQueueInsertLock.Why after the ProcArray removal, not before. The listener's gate is
XidInMVCCSnapshot(), so the invariant 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 lock tenure. A backend queued on the insert lock also no longer pins its xid/xmin, so it can't hold back the global horizon or delay visibility of an already-durable commit.Why an LWLock is a correctness requirement, not tidiness. Past the commit record, any
ERRORreachesRecordTransactionAbort()'s"cannot abort transaction %u, it was already committed"PANIC.LockSharedObject()can throw: out of shared memory for the lock table (a routine, user-reachable error), OOM, deadlock detection (which bypasses interrupt holdoff), andAcceptInvalidationMessages()running catalog callbacks.LWLockAcquire()allocates nothing, consults no lock table, runs no deadlock detector, and is released byLWLockReleaseAll()during error recovery. The earlier placement was also insideHOLD_INTERRUPTS(), making the wait uncancellable and unkillable —pg_terminate_backenda no-op. That's gone too.Measurements
4 vCPU,
fsync=on, zero listeners, 3 reps, 10 s, median [min–max].BEGIN; INSERT; NOTIFY; ENDvs the same transaction withoutNOTIFY:15.7x over stock at 64 clients, and commit latency 60.7 ms → 3.9 ms — indistinguishable from the NOTIFY-free control. Arm verification sampled zero backends waiting on either
Lock/objectorLWLock/NotifyQueueInsert, i.e. the insert lock is not observably contended.Note the v1 column: an earlier iteration that kept the heavyweight lock and inserted before
ProcArrayEndTransaction()reached only 10,965 at 64 clients. Moving past the ProcArray removal and switching to an LWLock is worth the remaining ~50%.Caveats: single 4-vCPU box, control cross-run variance is ~10% at 64 clients, and everything ≥8 clients is CPU-oversubscribed. Treat these as shape, not precision.
Known gaps — why this is not a patch
asyncQueueAddEntries()can still reachSlruReportIOError()(ENOSPConpg_notify/, read/close failures), which would PANIC post-commit. Needs either pre-faulting the target pages during pre-commit, or a decision to degrade toWARNINGand drop — whichasync.c's existing "notifications are not expected to survive database crashes" contract would justify. This is the gating item, and it's a semantics call for a committer.asyncQueueIsFull()stillereport(ERROR)s from the post-commit path. Needs capacity (not positional) reservation during pre-commit. Positional reservation is a dead end — it re-fixes queue order at reservation time and reintroduces the serialization.asyncregression test and all 130 isolation tests pass, but they do not appear to exercise two concurrent notifiers racing a listener's snapshot. Passing tests are not evidence the ordering is right — that rests on the argument above. An injection-point test is required.Relationship to upstream work
This is much smaller than Tom Lane's WAL-transport design (~1/20th), which Rishu Bagga has implemented to v9 with two commitfest entries currently in "Needs review". It is not a competing proposal — if the WAL transport lands it subsumes this. The interest here is that the throughput appears to come from relocating the serialized region rather than from moving notifications into WAL, which is a much smaller change to argue about.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Generated by Claude Code