Skip to content

notify-bench: harness isolating the NOTIFY commit lock (fork-local)#84

Draft
NikolayS wants to merge 3 commits into
masterfrom
claude/notify-bench-harness
Draft

notify-bench: harness isolating the NOTIFY commit lock (fork-local)#84
NikolayS wants to merge 3 commits into
masterfrom
claude/notify-bench-harness

Conversation

@NikolayS

Copy link
Copy Markdown
Owner

Fork-local infrastructure. Never intended for upstream. Everything here lives under notify-bench/ and would not be proposed to -hackers in this form. The upstream-bound patch is the documentation change in #83, which this PR supplies the supporting measurements for; background and analysis are in #82.

Why it exists

The obvious experiment — compare INSERT against INSERT; NOTIFY — does not isolate the lock. It measures the lock plus the SLRU queue insert, the utility statement itself, and an extra client round trip. Any speedup it shows is unattributable.

notify-bench/instrumentation/0001-MEASUREMENT-ONLY-skip-notify-commit-lock.patch fixes that. It makes the identical binary skip only the LockSharedObject() call in PreCommit_Notify() when PG_NOTIFY_NOLOCK is set in the postmaster environment. Both arms run the same executable, the same SQL, the same number of round trips, and insert the same queue entries; they differ by that one call.

The skip is semantically unsafe — it breaks the commit-order guarantee that the lock exists to provide. It is a measurement device, not a proposed optimization, and the patch is named accordingly.

Headline result

synchronous_commit=on, zero listeners, 3 reps, median [min-max] TPS:

clients control (INSERT) NOTIFY, lock held NOTIFY, lock skipped speedup from lock removal gap recovered
4 2,059 1,033 2,177 2.1x 111%
16 7,811 1,027 7,893 7.7x 101%
64 16,307 1,055 12,369 11.7x 74%

The gap recovered column is the point. If NotifyQueueLock — the SLRU queue lock — simply became the next bottleneck once the heavyweight lock was gone, removing the heavyweight lock would buy little. It does not: through 16 clients the no-lock arm reaches the NOTIFY-free control, and at 64 clients the residual is only ~26% of the gap. The commit-time lock, not the queue, is what bounds notifying throughput here.

The hardware-independent framing

Absolute TPS on a 4-vCPU box is not interesting. The wait-event data is, because it holds regardless of how fast the disk is: both arms run with exactly one fsync in flight at all times. The difference is what is queued behind it. The control arm has ~21 backends waiting on that one flush and group-committing with it; the NOTIFY arm has zero, because those backends are stuck a step earlier, waiting for the object lock and not yet in the commit path at all.

Group-commit batch factor ~21 versus exactly 1. That ratio is a property of the serialization, not of this storage.

Known defects — documented, not hidden

notify-bench/README.md enumerates these. The most important one invalidates a result:

  • The psql -f <fifo> listeners never call PQconsumeInput. They jam in ClientWrite instead of draining the queue. scripts/checkjam.sh demonstrates it directly: 50 of 50 listeners wedged. This flatters master and punishes v18 — that is, the bias runs in favour of the conclusion the harness was built to test, which is the worst direction for it to run. The "interested listener" results are therefore retracted. The uninterested-listener results, including the table above, do not depend on listener draining and are unaffected.

Also, in decreasing order of severity:

  • Single samples everywhere outside ab_lock.csv — no repetition, no dispersion.
  • The synchronous_commit=off rows sit at roughly a 50% noise floor; differences there are not meaningful.
  • A 4-vCPU box with pgbench and the listener processes co-resident with the server, so the load generator competes with what it measures.
  • No latency percentiles recorded, only throughput.
  • No synchronous replication configured, although sync rep is exactly the case where the serialized region is longest and the effect should be largest.
  • The test table uses a bigserial primary key, whose sequence and index contention cap the control ceiling — in both arms, so it compresses the measured gap rather than inflating it.

None of this is publication-ready. The README lists what has to be fixed before any of these numbers should be quoted outside this repository. It is checked in so the measurements behind #83 are reproducible and auditable, defects included.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW


Generated by Claude Code

claude added 2 commits July 25, 2026 06:42
Adds the benchmark harness, raw results and measurement instrumentation
used to characterize the cluster-wide lock that PreCommit_Notify()
takes.  Fork-local; not proposed for upstream in this form.

The central difficulty is that comparing INSERT against INSERT+NOTIFY
measures the lock plus the SLRU queue insert, the utility statement and
an extra round trip.  The instrumentation patch resolves this by making
the identical binary skip only LockSharedObject() when PG_NOTIFY_NOLOCK
is set in the postmaster environment, so the two arms differ by that one
call.  With synchronous_commit=on and zero listeners, removing it takes
throughput from 1,027 to 7,893 TPS at 16 clients and from 1,055 to
12,369 at 64 -- recovering essentially all of the gap to a NOTIFY-free
workload up to 16 clients and 74% of it at 64.  This shows the SLRU
NotifyQueueLock does not simply become the next bottleneck.

Stated without reference to this disk: the wait-event data shows both
arms with one fsync in flight, but the control queues ~21 backends
behind it while NOTIFY queues none, because they are stuck a step
earlier.  Group-commit batch factor ~21 versus exactly 1.

The README documents the harness's known defects rather than hiding
them, including one that invalidates the "interested listener" results
and whose bias favours the conclusion the harness was built to test:
psql reading from a fifo never calls PQconsumeInput, so those listeners
jam in ClientWrite instead of draining.  scripts/checkjam.sh
demonstrates it.  Those numbers are retracted; the uninterested-listener
results are unaffected.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Every context line of a unified diff is a space followed by the original
source line, which for this tree means a space followed by a tab.  The
stored instrumentation patch therefore trips "space before tab in
indent" and "trailing whitespace" in git diff --check, which SanityCheck
runs.

Add a directory-local .gitattributes clearing the whitespace attribute
for *.patch, mirroring what the root .gitattributes already does for
other files that legitimately contain special whitespace.  Scoped to
this directory so the upstream root file is untouched.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW

Copy link
Copy Markdown
Owner Author

CI: Windows - Visual Studio - Slice 2/2 failed. Not caused by this branch — diagnosis below so it isn't re-investigated.

The failure is a single test out of 198:

Summary of Failures:
 84/198 ecpg - postgresql:ecpg/ecpg    ERROR    21.19s   exit status 1

Ok: 180   Fail: 1   Skipped: 17

Two independent reasons it can't be attributable to this PR:

  1. The diff cannot reach ecpg. All 16 files are additions under notify-bench/ — shell scripts, CSVs, a README, a stored .patch, and a .gitattributes. Nothing is compiled, and nothing is referenced by meson or the makefiles. There is no path from this change to the ecpg test suite.
  2. The identical job passed on doc: document LISTEN/NOTIFY commit-time serialization #83, which is built from the same base commit (13b7a8a0) and likewise changes no C code.

So this is a flaky or environment-specific ecpg failure on the Windows VS runner, not a regression. SanityCheck — the job the second commit's .gitattributes was written to satisfy — passed, along with Linux Autoconf, Linux Meson 32-bit, macOS Meson and CompilerWarnings.

I tried to re-run the failed job immediately; the API rejected it with 403 This workflow is already running because other jobs in the run are still in flight. I'll re-run it once the workflow completes and update here only if it fails a second time — at which point it stops being a flake and needs real investigation.


Generated by Claude Code

Adds the two prototype arms measured on top of the lock-isolation result:
the v1 shape (heavyweight lock, insertion before ProcArrayEndTransaction)
and the v2 shape (dedicated LWLock, insertion after it), together with
their harness scripts, raw CSVs and the prototype diff.

v2 reaches 16,507 TPS at 64 clients against 1,055 for stock and 16,307
for a NOTIFY-free control, with commit latency falling from 60.7ms to
3.9ms.  v1 reached only 10,965, so the placement and lock choice account
for the remaining ~50%.

The README notes what these runs cannot show: with zero listeners
nothing ever reads the queue, so they cannot detect an ordering defect,
and control cross-run variance at 64 clients is ~10%.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01M3RLizCyeXBBFJdBHGDvKW
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants