Extended progression batches amortize the connection, not the transaction (2.39.2) - #630
Merged
Merged
Conversation
…ansaction marten#5167 reported that the extended-progression telemetry UPDATE increases database lock waits. It does, and the mechanism is not write volume: Marten implemented the batched overload as one `UPDATE ... FROM unnest(...)` statement, which takes a row lock on every shard in the batch and holds all of them until it commits. One slow projection batch sitting on one row therefore stalls the telemetry write of every OTHER shard on that database, and whatever queues behind those. Measured against PostgreSQL: an unrelated shard's progress write, contending with nothing, timed out after 4s queued behind a telemetry statement that had locked its row on the way to a different, genuinely contended one. Rewritten as one autocommit statement per row the same collision clears in ~1ms and only the genuinely contended row waits. That was a reasonable reading of the contract, which asked for "as few round-trips as the store can manage -- ideally one" and said nothing about transaction scope. It is now explicit that one row per transaction is required, that what a batch amortizes is the CONNECTION (which is all #553 ever needed -- N single-row statements on one rented connection cost one rent), and that implementations should skip rows whose telemetry is unchanged rather than hand every matched row a new tuple version per flush. The writer now also flushes batches ordered by shard name. With one row per transaction a single writer never holds more than one row lock, but the tracker is per-database and shared and building a daemon does not go through a cache, so two writers can race over the same rows -- and the pending dictionary's enumeration order is an implementation detail, not an agreement between them. Sorting removes that deadlock hazard for every store at zero cost. No behavior change for stores that never implemented the batched overload: the default interface implementation loops the single-state write, which already satisfies the rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CTtw2kVRSZKp1p5RTxgAy
This was referenced Aug 4, 2026
jeremydmiller
added a commit
that referenced
this pull request
Aug 4, 2026
A second ExtendedProgressionWriter on one database used to announce itself as lock contention -- two writers issuing multi-row UPDATEs over the same rows in plan-dependent order is a deadlock hazard. #630 made those writes one row per transaction in shard-name order, which makes a duplicate writer harmless to correctness and therefore SILENT: it just quietly does the same work twice on a second connection. That is worth knowing about, because the condition it indicates is real. The tracker is shared per database and building a daemon does not go through a cache, so a lifecycle bug can leave two STARTED daemons on one database, each arming its own writer. #621 gated arming so a daemon built only to READ state never subscribes one, which removed the common cause -- it did not remove the possibility. ShardStateTracker now logs a warning when an IExclusiveTrackerObserver attaches to a tracker that already has one of its role. Deliberately reported and not refused: the duplicate observer is the symptom, not the bug, and swallowing the subscription would hide the lifecycle bug rather than surface it. Unsubscribing removes the listener, so an ordinary daemon restart does not trip it -- pinned by its own test, since a warning that fires on every restart would be worth nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CTtw2kVRSZKp1p5RTxgAy
This was referenced Aug 5, 2026
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
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.
Follow-up to #622, driven by the lock analysis behind marten#5167.
The finding
The report that the extended-progression telemetry
UPDATEincreases database lock waits is correct, and the mechanism is not write volume. Marten implemented the batched overload as oneUPDATE … FROM unnest(…)covering the whole flush. A multi-row statement takes a row lock on every shard in the batch and holds all of them until it commits — so one slow projection batch sitting on one progression row stalls the telemetry write of every other shard on that database, and transitively whatever those shards were waiting to do.Reproduced against PostgreSQL with three sessions (P = a projection batch holding one row, H = the batched telemetry UPDATE over six, Q = an unrelated projection committing progress for a row P never touched):
Qtimed out after 4s. It was never contending withP— it was queued behindH, which had already lockedshard_aon its way toshard_dand then stalled there. ReplacingHwith six single-row autocommit UPDATEs,Qclears in ~1ms and onlyshard_d— the genuinely contended row — waits.This also explains the part of #5167 the reporter could not account for: that the blocker and the blocked statement were both the heartbeat UPDATE.
pg_blocking_pids()reports direct blockers only; the projection transaction that is the real root is one hop further down the chain.What changed
The contract is now explicit (
IEventDatabase.WriteExtendedProgressionAsync(IReadOnlyList<ShardState>, …)). It previously asked for "as few round-trips as the store can manage — ideally one" and said nothing about transaction scope, so folding the batch into one statement was a reasonable reading. It now states that one row per transaction is required, that what a batch amortizes is the connection — which is all #553 ever needed, since N single-row statements on one rented connection cost one rent — and that implementations should skip rows whose telemetry is unchanged rather than hand every matched row a new tuple version per flush.Batches flush ordered by shard name. With one row per transaction a single writer never holds more than one row lock, but
Trackeris per-database and shared and building a daemon does not go through a cache, so two writers can race over the same rows. The pending dictionary's enumeration order is an implementation detail, not an agreement between them; sorting removes that deadlock hazard for every store at zero cost. Pinned bya_flushed_batch_is_ordered_by_shard_name.No behavior change for stores that never implemented the batched overload — the default interface implementation loops the single-state write, which already satisfies the rule. The Marten-side rewrite is marten#5186.
Verification
Full
EventTestssuite green (725 tests) on net9.0.🤖 Generated with Claude Code
https://claude.ai/code/session_017CTtw2kVRSZKp1p5RTxgAy