Skip to content

Index inbox receipts by codec, and keep inbox reads off full scans - #210

Draft
MaxGhenis wants to merge 1 commit into
mainfrom
perf/inbox-codec-received-index
Draft

MaxGhenis wants to merge 1 commit into
mainfrom
perf/inbox-codec-received-index

Conversation

@MaxGhenis

Copy link
Copy Markdown
Owner

Summary

InboxReceiptsBetween (silence baselines in internal/freshness, and #199's gap walks) ran SCAN inbox plus a temp B-tree sort, because the only index on received_at_ms is partial (WHERE processed_at_ms IS NULL). The inbox never sheds rows, so that cost grows for as long as the store exists. This PR:

  1. Migration 0011 inbox_codec_received: CREATE INDEX inbox_codec_received_idx ON inbox(codec, received_at_ms). Production passes one codec per platform (cmd/serve.go maps google.protobuf, whatsapp.event and signal.jsonrpc one-to-one), so the baseline read becomes a covering-index range search, already in receipt order.
  2. InboxReceiptsAfterRow keeps its rowid range. I checked its GROUP BY as asked, and the index alone would have regressed it. Offered the new index, SQLite scans the whole index to skip the GROUP BY sort, so the incremental refresh behind /api/status (recomputed at most every 30 s while the app polls) went from a rowid range search over a few new rows to a scan of every row. That's 0.02 → 50 ms at 1M rows, on every refresh. The incremental form now reads FROM inbox NOT INDEXED WHERE rowid > ?; SQLite still allows the rowid lookup under NOT INDEXED. The full rescan (afterRowID <= 0: every 6 h, on the first refresh after the daemon starts, and on every openmessage status run) now walks the index per codec (a recursive-CTE skip-scan) instead of reading every table row.
  3. AppendInbox's dedupe lookup was also a full scan, found by the same planner check. inbox_dedupe_uq is partial (WHERE dedupe_key <> ''), and WHERE account_id = ? AND dedupe_key = ? with a bound key can't prove that condition, so SQLite scanned the inbox for every re-delivered frame. A history catch-up that re-fetches a frame already on file takes this path (history_deduped in internal/ingest/sink.go), so re-running a window backfill paid one full scan per duplicate. The lookup only runs for a nonempty key, so it now adds AND dedupe_key <> '' (same result) and uses the unique index. Live counters since this morning's restart show 0 dedupes, so today the cost is per catch-up, not continuous.

The query texts moved into package constants (inboxReceiptsAfterRowQuery, inboxLatestReceiptByCodecQuery, inboxReceiptsBetweenQuery, inboxDedupeLookupQuery) so tests can pin their plans. The 10 → 11 edits in the migration-count assertions and the internal/migration/validate.go checksum pin follow the pattern #196 uses.

Evidence: copies of the live store

Measured on a read-only cp of ~/Library/Application Support/OpenMessage/v2/store.sqlite3 (+ -wal/-shm; the WAL hash was identical before and after the copy, and integrity_check = ok). The live store was never opened. Store facts: schema v10, 30,254 inbox rows (Google 9,155, Signal 13,943, WhatsApp 7,156), 37 MB table, no sqlite_stat1. To size growth I rebuilt the inbox at 10× and 33× (302,540 and 998,382 rows) by repeating it back in time, inserted in receipt order so rowids track time as they do live, with full payloads (pre-#196 worst case). Each query ran through the repo's own driver (modernc.org/sqlite v1.44.3 = SQLite 3.51.2) via a throwaway test. Times are medians of 15 warm runs; the machine's load average was about 150–200 from other sessions, so treat absolute times as upper bounds.

query 30k before 30k after 302k before 302k after 998k before 998k after
Silence baseline, Google 14 d (InboxReceiptsBetween) 10.72 0.34 69.17 0.38 294.42 0.36
Gap walk, 2 h window 9.83 0.01 66.85 0.01 241.85 0.01
InboxReceiptsAfterRow incremental (each status refresh) 0.02 0.02 0.03 0.04 0.02 0.02
  same, if the index were added and the query left as is – 1.04 – 12.96 – 50.47
InboxReceiptsAfterRow(0) full rescan 18.90 0.03 343.83 0.02 972.29 0.04
AppendInbox dedupe lookup (each re-delivered frame) 11.43 0.01 74.08 0.01 278.64 0.01
#191 ListInboxByCodecSince 45 d (not on main) 80.07 17.27 200.27 18.58 413.95 18.19

EXPLAIN QUERY PLAN, before → after (identical at every scale):

query before (main) after (this PR)
InboxReceiptsBetween (1 codec) SCAN inbox / USE TEMP B-TREE FOR ORDER BY SEARCH inbox USING COVERING INDEX inbox_codec_received_idx (codec=? AND received_at_ms>? AND received_at_ms<?)
InboxReceiptsAfterRow, incremental SEARCH inbox USING INTEGER PRIMARY KEY (rowid>?) / temp B-tree for GROUP BY unchanged (with the index and the old text: SCAN inbox USING COVERING INDEX inbox_codec_received_idx)
InboxReceiptsAfterRow(0) SEARCH … (rowid>?) over every row / temp B-tree SEARCH … COVERING INDEX inbox_codec_received_idx (codec>?) and (codec=?) per codec, plus one MAX(rowid) seek
dedupe lookup SCAN inbox SEARCH inbox USING INDEX inbox_dedupe_uq (account_id=? AND dedupe_key=?)
#191 ListInboxByCodecSince SCAN inbox / temp B-tree SEARCH … inbox_codec_received_idx (codec=? AND received_at_ms>?) / TEMP B-TREE FOR LAST TERM OF ORDER BY
Unprocessed, by-id lookups, mark-processed UPDATE unchanged unchanged

With ANALYZE statistics present (the store never runs ANALYZE; checked at 302k rows), every plan above stays the same.

Index shape. (codec, received_at_ms): 880 KiB at 30k rows, 9.0 MB at 302k, 29.9 MB at 998k (about 30 bytes a row). I also measured (codec, received_at_ms, inbox_id). It removes #191's tie-break sort (15.6 → 12.2 ms at 30k) but is 2.1× the size (1,880 KiB at 30k), since inbox_id is 32 hex characters, and nothing on main needs it.

Migration at startup

The daemon opens and migrates the v2 store in newV2Stack before its HTTP server starts. The app polls /api/status for up to 30 s (BackendManager.startHealthCheck), so the migration delays readiness by its own length and nothing more. applyMigration runs in a BEGIN IMMEDIATE transaction, and runs PRAGMA foreign_key_check after the SQL.

inbox rows Open() with 0011 pending CREATE INDEX alone (3 runs) foreign_key_check alone
30,254 (live) 0.61 s 0.21–0.29 s 0.42–0.55 s
302,540 0.79 s (1.93 s under load) 0.92–1.19 s 0.27–0.36 s
998,382 3.37 s 2.75–3.82 s 0.43–0.49 s
  • It runs once: the ledger records 0011 and a reopen applies nothing (tested).
  • Readers in other processes keep reading the pre-migration snapshot under WAL.
  • Another process that opens the store during the migration waits on the write lock up to busy_timeout (5 s). That's well clear at today's 30k rows. Extrapolating the timings above, it would take somewhere past 1.2M–1.7M unpruned rows on this machine.
  • The index build writes about the index's size to the WAL (about 30 MB at 1M rows).

Deploy lockstep (same as #196). serve --mcp-stdio, read and status open the v2 store with sqlite.Open, which migrates. After the app's daemon applies 0011, an older /usr/local/bin/openmessage (the one ~/.mcp.json runs) fails v2 reads with database schema version 11 is newer than supported version 10. Install the CLI together with the app. If the CLI is updated first, it migrates the live store under the running v10 daemon. That daemon keeps running, though its unchanged InboxReceiptsAfterRow then scans the new index on each refresh (about 1 ms at today's size), and its next restart fails the same way.

Coordination

#196 (inbox_payload_retention), #201 (remote_idspace_epochs) and #166 (outbox_expiry) each also add a migration numbered 0011. Whichever merges after this one renumbers to the next free number, re-pins internal/migration/validate.go, and bumps the count assertions; I'll comment on each. Checked against #196 specifically:

#191's ListInboxByCodecSince (414 → 18 ms at 1M rows) picks up the index with no change.

Invariants (stated, then tested)

  • InboxReceiptsBetween(codecs, from, to) returns exactly the receipts of rows whose codec is listed and whose received_at_ms lies in [from, to]. They come back ascending, with duplicates kept.
  • InboxReceiptsAfterRow(after) returns two things:
    • for each codec, the newest receipt among rows with rowid > after (every row when after <= 0), with codecs that have no such row left out;
    • max(after, largest such rowid).
  • The full-rescan form and the rowid-range form agree on the same inbox.
  • Every answer is identical with inbox_codec_received_idx dropped. The index changes plans, not results.
  • Re-appending a dedupe key returns the first frame's inbox_id and adds no row.
  • The migration leaves every inbox row byte-identical and the 0001–0010 ledger untouched, and applies once.

Tests:

  • TestInboxActivityQueriesMatchReferenceProperty: 80 testing/quick cases with a fixed seed. Frames are appended out of time order, with many tied receipts, random windows and codec subsets, and re-deliveries. Each case is checked against a Go reference, against the other query form, and with the index dropped.
  • TestInboxQueryPlansAvoidFullScans pins the plans above.
  • TestInboxCodecReceivedMigrationAppliesToBlankAndExistingV10Database covers a blank store and a populated v10 store: rows unchanged, index columns and order, pinned checksum, idempotent reopen.
  • TestInboxActivityQueriesOnEmptyInbox.
  • Mutation check: all 7 mutants were killed. They were: drop NOT INDEXED, drop dedupe_key <> '', MAX→MIN in the rescan, send after=1 down the rescan path, drop the rescan's IS NOT NULL guard, swap the index columns, and drop ORDER BY.

Ran (targeted, GOWORK=off): go test ./internal/storage/sqlite/ ./internal/migration/ ./internal/freshness/ ./internal/ingest/, go test ./cmd/ -run 'Silence|Status|Freshness|Inbox|V2Stack|Migrat', and go test ./internal/web/ -run 'Silence|Status|Freshness'. All passed.

Evidence (raw matrices, the ANALYZE run, the #196 combination, migration timing, mutant script, scale script, diagnostic test): ~/reviews/openmessage-inbox-codec-index-2026-10-09/.

🤖 Generated with Claude Code

Silence detection reads one codec's receipt times inside a window, and the
only index on received_at_ms was partial (unprocessed frames), so every
baseline read scanned the whole inbox and sorted it. Migration 0011 adds
inbox(codec, received_at_ms), which answers that read from the index alone.

With the index present SQLite scanned all of it for the incremental
InboxReceiptsAfterRow refresh (to skip the GROUP BY sort), so that query
now pins the rowid range with NOT INDEXED, and the full rescan steps through
the index per codec instead of reading every row. The AppendInbox dedupe
lookup restates the partial unique index's condition so it uses
inbox_dedupe_uq instead of scanning the inbox.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering: #215 (SQL plan audit) also adds a migration numbered 0011 (read_path_indexes: three indexes, no row changes), as do #196, #201, #166 and #210. Whichever merges first keeps 0011 and the rest renumber to the next free version; the version-count lines every migration PR edits (internal/storage/sqlite/*_test.go, internal/migration/validate.go, internal/migration/transform_test.go) move with it. #215 finds its own migration by name in its tests, so renumbering it is mechanical.

No statement overlap: #215 leaves the inbox dedupe lookup and the receipts reads to this PR and lists them as fixed here.

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering: #223 (FTS5 trigram substring search, stacked on #215) adds migration 0012 substring_search after #215's 0011. Same rule as above: whichever lands first keeps its number and the rest renumber. #223's tests find its migration by name; its version-count lines and internal/migration/validate.go's version/checksum gate (now the validationPassed function) move with it.

@MaxGhenis

Copy link
Copy Markdown
Owner Author

Migration numbering update: #215 merged to main (29a9a8e) and took 0011 (read_path_indexes: three indexes, no row changes), so this PR's migration needs the next free number. Renumber it to 0012, or later if another lands first. That touches:

  • the SQL file name, plus the //go:embed and newMigration lines in internal/storage/sqlite/migrations.go;
  • the count lines (want 11 → want 12) in internal/storage/sqlite/*_test.go and internal/migration/transform_test.go;
  • the internal/migration/validate.go pin, which must name this PR's own migration and checksum as the last one.

Two more things changed on main:

  • #218's rebuild fixture in migrations_fk_test.go now recreates every messages index, including 0011's two.
  • Read clients (read/status, the MCP client) and repair dry runs now open the store with sqlite.OpenWithoutMigrating. Any test that expected those clients to migrate a store must provision it at the current version first.

🤖 Generated with Claude Code

This branch has not been deployed

No deployments
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.

1 participant