fix(server): bound inbox ACK cost to the partition's non-acked rows - #2110
Draft
bestony wants to merge 2 commits into
Draft
fix(server): bound inbox ACK cost to the partition's non-acked rows#2110bestony wants to merge 2 commits into
bestony wants to merge 2 commits into
Conversation
`ackThroughEntryIdForBoundAgents` selected - and, under `FOR UPDATE`, locked - every notify=true row in the `(inbox_id, chat_id)` partition below the ACK cursor, including rows acked long ago, then materialised them in JS. Cost per ACK was O(chat history) and O(N^2) over a chat's life; a duplicate ACK that commits nothing paid the same price. The cause was not a missing index. `idx_inbox_chat_silent` is (inbox_id, chat_id, notify, status) and the planner was already using it - the query simply never constrained `status`, so a four-column index was used as a three-column one and `id <= cursor` became a heap filter over the whole partition. Already-acked rows cannot change any of the three decisions this commit makes: they never form a prefix gap, are never committable, and are never reset-from-pending. Excluding them is an exact equivalence, not an approximation, and it holds because `acked` is terminal for status transitions - every UPDATE here is guarded on 'pending' or 'delivered', and the only statement touching acked rows is the GC DELETE, restricted to notify=false and therefore disjoint from this notify=true scan. Spelled as a positive IN because `status <> 'acked'` is not sargable: measured on the same dataset it stays a filter, reads 168,020 rows and saves only the row locks. The status set is derived from the shared domain minus the terminal value rather than hardcoded, so a future fourth status keeps being scanned and can still reject a commit as a gap; a literal pair would start skipping it and let the ACK through. The domain has changed once already (a legacy `failed` value cleared by migration 0066) and its CHECK constraint was added NOT VALID. No schema change, no new index, no migration. Measured end-to-end through the real service call, median of 40 samples, on both major versions this repo runs (docker-compose uses postgres:16-alpine, CI uses postgres:17): history before after (17.10) after (16.14) 50,000 119.6 ms 1.46 ms 2.53 ms 150,000 341.4 ms 2.25 ms 2.77 ms 300,000 545.0 ms 2.66 ms 2.61 ms Flat instead of linear; the residual is the transaction's fixed cost, which an empty-history ACK also pays. At the query level the duplicate ACK goes from 168,000 rows / 2,937 buffers to 0 rows / 4 buffers, and with FOR UPDATE from 338,980 buffers / 2,800 dirtied pages to 44 / 0. `LockRows` still sits above the `Sort`, so rows are still locked in ascending id order. Cost now tracks the number of non-acked rows in the partition rather than the history. `id <= cursor` remains a filter, so a large pending backlog still costs proportionally to that backlog. Refs #1671
…y limit Two review follow-ups, comments only. The status restriction only reaches the index condition under a custom plan on PostgreSQL 16. It is stable under the default plan_cache_mode on 16.14 and 17.10 because the generic estimate is far more expensive, but a deployment forcing force_generic_plan globally would silently return the scan to O(history) there; 17 is unaffected. Worth stating next to the clause rather than only in review. The cost guard replays the captured SQL after the ACK has committed, so a correct implementation matches close to zero rows instead of the handful it saw live. It still separates the two cases -- a query without the restriction reads the whole partition on replay -- but the absolute number is not the live scan, and the next reader should not mistake it for one. Refs #1671
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.
Fixes the quadratic ACK cost described in #1671 (PERF-008).
The problem
ackThroughEntryIdForBoundAgentsselected — and, underFOR UPDATE, locked — everynotify=truerow in the(inbox_id, chat_id)partition below the ACK cursor, including rowsacked long ago. Those rows were then materialised as JS objects. Cost per ACK was O(chat
history) and O(N²) over a chat's lifetime, and a duplicate ACK that commits nothing paid the
full price.
Measured end-to-end through the real service call (not just the SQL), on a migrated database
with the production index set:
Roughly 2 µs per row of history, strictly linear.
Root cause
Not a missing index.
idx_inbox_chat_silent (inbox_id, chat_id, notify, status)alreadyexists and the planner was already using it — the query just never constrained
status, so afour-column index was used as a three-column one. Every row of the partition came back and
id <= cursorwas re-checked as a heap filter:The locking cost was larger than the scan: with
FOR UPDATE, that same statement touched338,980 buffers and dirtied 2,800 pages, purely to take row locks on rows that were already in
their terminal state.
The change
One clause on the prefix scan, restricting it to the statuses an ACK decision can still be
changed by. No schema change, no new index, no migration.
Why it is an exact equivalence
The prefix rows feed exactly three decisions, and an acked row contributes to none of them:
status !== 'acked' && status !== 'delivered' && !isResetDeliveredstatus === 'delivered'status === 'pending' && deliveredAt !== nullThis holds because
ackedis terminal for status transitions. EveryUPDATEagainstinbox_entriesis guarded on'pending'or'delivered'; the only statement that touchesacked rows at all is the GC
DELETEinpruneStaleSilentEntries, which is restricted tonotify=falseand therefore disjoint from thisnotify=truescan.The ACK expansion set is unchanged — same inbox, same chat, same cursor. The scan simply stops
reading members of it that cannot affect the outcome.
Why a positive
INrather than<> 'acked'<>is not sargable. Measured on the same dataset,status <> $4stays a filter and removes168,000 rows after reading them — it saves the row locks and nothing else:
status <> $4status IN ($4, $5)Why the status set is derived rather than written out
ACK_PREFIX_SCAN_STATUSESis computed from the shared status domain minus the terminal value.The failure direction is what matters. If a fourth status is ever added, a derived set keeps
scanning it, so an unknown status still rejects the commit as a prefix gap — the same
fail-closed behaviour the original status-agnostic query had. A hardcoded
["pending", "delivered"]would instead skip those rows and let the ACK through, turning asafe rejection into silent data loss. This is not hypothetical: the column's domain has changed
once already (a legacy
failedvalue, normalised by migration0066), andck_inbox_entries_statuswas addedNOT VALID.Results
End-to-end service call, median of 40 samples after warm-up, on both PostgreSQL major
versions this repo runs (
docker-compose.ymlusespostgres:16-alpine, CI usespostgres:17):Flat instead of linear. The residual ~2.5 ms is the transaction's fixed cost — an ACK against a
chat with no history measures 3.0–3.2 ms — so history no longer contributes at all.
Query level, same dataset, verified on 16.14 and 17.10 through the real driver (postgres-js
named prepared statements, repeated executions, default
plan_cache_mode):FOR UPDATEchat_id IS NULLpartitionLockRowsstill sits above theSort, so rows are still locked in ascending id order and thelock ordering between concurrent ACKs on one partition is unchanged. The rows that stop being
locked are exactly the acked ones, which no production statement mutates.
Operational caveat
On PostgreSQL 16 the status restriction only reaches the index condition under a custom
plan. That is what the default
plan_cache_mode = autoproduces here, stably: the genericestimate is hundreds of times more expensive, so the planner keeps rejecting it — verified over
repeated executions and across a plan-pollution sequence (same prepared statement serving a
small partition first, then the large one). But a deployment that sets
plan_cache_mode = force_generic_planglobally would silently return this scan to O(history) on16. PostgreSQL 17 uses the bound parameters as an index condition either way. Noted in the code
next to the clause.
Scope of the win, stated precisely
Cost now tracks the number of non-acked rows in the partition, not the cursor position and
not the history.
id <= cursoris still a filter, so a partition holding a largependingbacklog (an agent offline for a long time —
deliveredis capped per chat,pendingis not)still costs proportionally to that backlog. Draining such a backlog one ACK at a time remains
O(N²) in the backlog size. That is one to two orders of magnitude better than the previous
behaviour and removes the history dependence #1671 is about, but it is not O(1).
Alternatives considered and rejected
A per-
(inbox, chat)high-water cursor (the issue's first suggestion) is not merelyheavier — it is incorrect here.
inbox_entries.idcomes from a sequence allocated at INSERTtime, while the fan-out INSERT in
services/message.tshappens before the only serialisationpoint in that transaction (
UPDATE chats SET updated_at). Commit order therefore does notfollow id order. Reproduced with two sessions: A allocates id 1 and holds its transaction open,
B allocates id 2 and commits; an observer at that instant sees only id 2. A watermark advanced
to 2 would leave id 1 permanently below it once A commits — the client's ACK for row 1 would
hit the "already acked" short circuit, the row would stay
deliveredforever, bind-timerecovery would reset it to
pending, redeliver, and short-circuit again. An unboundedredelivery loop. Today's code self-heals in that scenario. Closing the hole would require the
delivery path to walk the watermark backwards — a new invariant spanning delivery, ACK and
recovery, plus a table and a migration.
Extending
idx_inbox_chat_silentwith a trailingidwould makeid <= cursoran indexcondition. Measured: the planner keeps choosing the existing four-column index even when the
five-column one is available, and that index costs 147 MB against 19 MB on a table that sits on
the message fan-out write path. Not worth it.
Rewriting the three decisions as pure SQL to remove the JS materialisation: once the scan
is bounded to the non-acked window, materialising it is negligible. A larger diff for no
measurable gain.
Not adding a migration is a feature here
inbox_entriesalready carries five indexes plus a unique constraint on the fan-out write path.Beyond that, CI pins
packages/server/drizzle/LATESTto the latest journal tag specifically soracing PRs conflict — #2108 landed while this was in progress and took slot
0091. A migrationin this PR would have needed rebasing; there is nothing here to rebase. Drizzle migrations also
run inside a transaction, so
CREATE INDEX CONCURRENTLYis unavailable and any new index wouldhold a lock on a large table for the duration.
Tests
packages/server/src/__tests__/inbox-ack-prefix-scan.test.ts:service actually emits, and replays each read against
EXPLAIN (ANALYZE). Asserts the widestscan does not grow when the history grows 5×. It asserts on the service's own statements
rather than a copy of the query, so inlining the query back into the service or dropping the
predicate both surface. Fault-injected: removing the status clause turns it red
(
expected 1501 to be less than or equal to 309). The replay happens after the ACK hascommitted, so a correct implementation matches close to zero rows rather than the handful it
saw live — what is asserted is the presence of the restriction, not the live row count.
value, and cross-checks the shared enum against the live
CHECKconstraint so a database-sidedomain change cannot drift away unnoticed.
chat_id IS NULLpartition (theisNullbranch ofchatPredicate, previously uncovered), and a prefix gap sitting above along acked history still rejected.
Equivalence was additionally checked exhaustively in SQL before implementation: every
(partition × cursor) combination over an adversarial dataset — acked rows interleaved before,
between and after every interesting row, the NULL-chat partition, recovery-reset rows,
cross-inbox interference, gaps on either side of the cursor — derived all three decisions both
ways and diffed them. 60 cases, 0 disagreements; a deliberately broken variant produced 29,
confirming the check has detection power.
pnpm check,pnpm typecheckand thepackages/serversuite pass.