mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-30 15:48:36 +00:00
Two-reviewer adversarial audit of the branch found no correctness/data-loss/ crash bugs. This closes one gap and applies three robustness fixes: - Multi-filter search ordering: a REQ whose filters all carry a search term (e.g. the client's search-across-kinds) was created_at-ordered via the union path. Now relevance-ordered — unionSubqueriesIfNeeded(projectRank) projects rank per branch, UNION ALL + GROUP BY row_id MIN(rank) dedups across branches keeping the best score. Only when every branch is a search branch; mixed search/non-search REQs and count/delete unions stay as before. - prepareAuthorStreams/prepareTagStreams build cursors via buildStreams, which closes already-prepared statements if a later prepare throws (was: stranded checked-out, un-reset handles holding read locks in the pooled connection). - Stream counts computed as Long so authors×kinds / values×kinds can't overflow Int back into the eligible band and route a huge fan-out into the merge. - Renamed CachedStatement.finalize() -> finalizeStatement(): a no-arg finalize() is the JVM Object.finalize, risking a GC double-close of the native handle. Tests: multi-filter search relevance + cross-branch dedup + count parity; existing merge/cache/search suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqGuBuSUsRudGerPqDuoKA
169 lines
9.7 KiB
Markdown
169 lines
9.7 KiB
Markdown
# SQLite query scaling: contentless FTS + rowid search, tag-path merge, pooled statements
|
||
|
||
**Status: shipped.** Follow-up to a scale-curve report (`SQLite vs Vespa`,
|
||
throughput vs corpus size) showing the app-side SQLite store degrading with
|
||
corpus size on several query shapes — NIP-50 search ~18× (25k→400k), batch
|
||
ingest 16×, author-timeline 13×, follow-feed 2.4× — while point reads stayed
|
||
flat. Two of those (author-timeline, follow-feed) were already addressed
|
||
(the pubkey-alone index and `MergeQueryExecutor`) and mostly reflect running
|
||
the report under the *client* `DefaultIndexingStrategy` rather than
|
||
`relayIndexingStrategy()`. This change adds the **large-IN tag watcher** to the
|
||
merge executor, fixes the **FTS delete path** (which degraded with corpus
|
||
size) and shrinks the FTS index, fixes **NIP-50 search ordering** (it was
|
||
sorting by `created_at`, not relevance), and fixes a **statement-cache** miss
|
||
the merge paths hit.
|
||
|
||
It does **not** fix NIP-50 *search* latency: bm25 relevance scoring (like the
|
||
old `created_at` sort) must visit every match, so search cost still grows with
|
||
the match set (§1). Corpus-independent search is an external-engine job.
|
||
|
||
Everything here is read/size work; the write path and on-disk index set are
|
||
unchanged except the FTS table, which gets *smaller* and deletes faster.
|
||
|
||
## 1. FTS — contentless index (fast deletes + smaller), NOT a search-scaling fix
|
||
|
||
The old index was `fts5(event_header_row_id, content)`: it stored a second
|
||
copy of the tokenized text, its rowid was auto-assigned (unrelated to the
|
||
event), and — the real problem — the `fts_foreign_key` delete trigger deleted
|
||
by the `event_header_row_id` *column*, which FTS5 cannot seek, so it **scanned
|
||
the whole index per delete**.
|
||
|
||
Changes (`FullTextSearchModule`, `QueryBuilder`, DB version 4→5):
|
||
|
||
- **rowid = `event_headers.row_id`.** Set explicitly on every insert; the
|
||
delete trigger, reindex, and catch-up paths key off it. Deletes become an
|
||
O(log n) primary-key seek instead of an O(n) column scan. This is the win:
|
||
every event removal fires the trigger — replaceable rotation, kind-5,
|
||
expiration, right-to-vanish — so on the old schema deletion throughput
|
||
degraded with corpus size.
|
||
- **Contentless** (`fts5(content, content='', contentless_delete=1)`). The
|
||
indexed text is *derived* (`SearchableEvent.indexableContent()`, not a raw
|
||
column), so FTS5 **external-content** — which re-reads the source column from
|
||
the base table — cannot express it; **contentless** is the correct primitive.
|
||
It drops the stored content copy (index shrinks) and `contentless_delete=1`
|
||
keeps the delete trigger working.
|
||
- **Segment compaction.** `reindexAll` finishes with `'optimize'`, and the
|
||
periodic `SQLiteEventStore.optimize()` (geode's maintenance tick) folds in a
|
||
bounded `'merge'`, so incremental / deferred-catch-up inserts don't leave the
|
||
index as many small segments.
|
||
|
||
Delete cost — `FtsSearchScalingBenchmark.deleteByColumnVsByRowid`, 500 deletes,
|
||
in-memory:
|
||
|
||
| rows | by column (old) | by rowid (new) |
|
||
|---|---:|---:|
|
||
| 2k | 91.8 ms | 6.6 ms |
|
||
| 8k | 362.0 ms | 4.7 ms |
|
||
|
||
By-column grows ~linearly with the table (O(n)/delete); by-rowid is flat —
|
||
~78× at 8k rows and widening.
|
||
|
||
**Search ordering fixed to relevance (NIP-50), which the store was getting
|
||
wrong.** NIP-50 says results are returned "in descending order by quality of
|
||
search result ... not by the usual `.created_at`", with the limit applied after
|
||
the score — but the store sorted search by `created_at DESC` (pre-existing).
|
||
*Every* search filter now orders by FTS5 bm25 (`ORDER BY event_fts.rank`,
|
||
`created_at DESC` as a tie-break): the tag-free shape (`makeSimpleSearch`) and
|
||
`search + tag` (the `prepareRowIDSubQueries`/`makeQueryIn` path, which carries
|
||
the rank column through the row-id subquery via `projectRank` and applies the
|
||
LIMIT by rank). Only the negentropy snapshot keeps `created_at` — a sync set,
|
||
not a ranked result. Verified against stronger-but-older matches outranking
|
||
weaker-but-newer ones, tag-scoped, with the limit cutting by score
|
||
(`SearchRelevanceOrderTest`, `Fts5CapabilityProbe`).
|
||
|
||
An earlier draft of *this* change instead added a `searchOrderByRowId` flag
|
||
(order by the FTS rowid for O(limit) search); that is *ingestion* order — wrong
|
||
events under a limit once ingestion diverges from time order, and not relevance
|
||
either — so it was removed.
|
||
|
||
This is a **correctness** fix, not a scaling one: bm25 (like the created_at
|
||
sort) must score every match, so search latency still grows with the match set
|
||
(the report's 18× curve) and `optimize` doesn't change the asymptotics
|
||
(measured within noise at 100k/200k). Corpus-independent search is genuinely an
|
||
external-engine job (the Vespa side of the report), not this index.
|
||
|
||
Migration (v4→v5): the old rowids can't be remapped, so `event_fts` is dropped
|
||
and rebuilt — synchronous stores rebuild in the upgrade transaction (client
|
||
corpora are small), deferred stores reset the catch-up watermark to 0 and let
|
||
the background worker repopulate (no long migration transaction).
|
||
`ContentlessFtsMigrationTest` fabricates a real v4 DB (old schema + garbage
|
||
rows + `user_version=4`) and asserts the reopen rebuilds search, wipes the
|
||
stale rows, maps rowid→row_id, and keeps the delete trigger working.
|
||
|
||
## 2. Tag watcher — extend `MergeQueryExecutor` to the tag path
|
||
|
||
`kinds=[7] AND #e=[hundreds of note ids] LIMIT n` (reactions/replies) had the
|
||
same shape the follow-feed fix already solved for authors: the per-value
|
||
streams come sorted off `(tag_hash[, kind], created_at)`, but their union does
|
||
not, so SQLite collected every matching row and TEMP-B-TREE-sorted to the limit
|
||
— O(matching history), growing with the corpus (`TagAuthorIndexBenchmark`:
|
||
12.8 ms cold at 200k → 14.2 ms at 1M).
|
||
|
||
`MergeQueryExecutor` now opens one lazy newest-first cursor per `(value[,
|
||
kind])` off `query_by_tags_hash_kind` (or `query_by_tags_hash` when there is no
|
||
kind, gated by `indexTagsByCreatedAtAlone`) and heap-merges to the limit —
|
||
**O(limit + streams)**. Unlike the author path, one event can carry several
|
||
queried tag values, so the tag merge **dedups by event id** through a `seen`
|
||
set (the author path, one pubkey per event, skips it). Eligibility is narrow
|
||
(one non-`d` tag key, ≥2 values, a limit, no ids/authors/d-tag/search, no
|
||
AND-tags); everything else falls through to the single-SQL plan. Counts and
|
||
deletes are unchanged (still single-SQL). `TagMergeCorrectnessTest` pins it
|
||
against a Kotlin reference including cross-stream dedup, since/until windows,
|
||
the raw path, and tie handling (tag cursors order off `event_tags`, which has
|
||
no id column, so same-second ties are a valid newest-N but not id-exact).
|
||
|
||
## 3. Pooled statement cache — so the merges actually cache
|
||
|
||
`StatementCachingConnection` kept **one** handle per SQL string. The k-way
|
||
merge opens *many* identical-SQL cursors at once (one per author/tag stream),
|
||
so every stream past the first missed the cache and prepared uncached, and a
|
||
repeated follow-feed / reactions REQ re-prepared all of them each poll. The
|
||
cache now keeps a small **pool** per SQL (bounded by a global cap, default
|
||
raised 256→512), so concurrent same-SQL checkouts reuse cached handles and
|
||
repeated polls reuse their per-stream cursors. `StatementCachingConnectionTest`
|
||
covers sequential reuse, concurrent distinctness/independence, freed-handle
|
||
reuse, and cap overflow → uncached fallback.
|
||
|
||
## Correctness / scope
|
||
|
||
- DB version bump 4→5 with a real-upgrade test.
|
||
- New capability test (`Fts5CapabilityProbe`) pins the FTS5 features the
|
||
contentless index needs (contentless_delete, absent-rowid delete no-op,
|
||
rowid ordering, merge/optimize) to the bundled SQLite (3.50.1), so a driver
|
||
downgrade fails loudly.
|
||
- All existing `store.sqlite` tests pass unchanged except `QueryAssemblerTest`,
|
||
whose asserted EXPLAIN output updated for the new join column
|
||
(`event_fts.rowid`) and the contentless table's virtual-index marker
|
||
(`0:M2`→`0:M1`).
|
||
- Search ordering: bm25 relevance for every search REQ shape — tag-free,
|
||
`search + tag`, and multi-filter all-search (e.g. the client's
|
||
search-across-kinds, unioned then deduped by event keeping the best score).
|
||
Mixed search/non-search multi-filter REQs stay `created_at` (a non-search
|
||
branch has no defined relevance).
|
||
|
||
## Audit follow-ups (post-review hardening)
|
||
|
||
A two-reviewer adversarial audit of the branch found no correctness/data-loss/
|
||
crash bugs; it produced these robustness fixes and one gap-closure:
|
||
|
||
- **Multi-filter search ordering** (gap): a REQ of several filters that all
|
||
carry a search term was `created_at`-ordered (the union path). Now
|
||
relevance-ordered via `unionSubqueriesIfNeeded(projectRank)` — each branch
|
||
projects `rank`, `UNION ALL` + `GROUP BY row_id MIN(rank)` dedups across
|
||
branches keeping the best score. Only when every branch is a search branch
|
||
(and FTS is on); count/delete unions stay single-column.
|
||
- **Merge stream-prep leak** (F1): `prepareAuthorStreams`/`prepareTagStreams`
|
||
now build cursors through `buildStreams`, which closes any already-prepared
|
||
statements if a later prepare throws — otherwise a mid-loop failure stranded
|
||
checked-out, un-reset handles (read locks) in the pooled connection.
|
||
- **Stream-count overflow** (F3): `authors×kinds` / `values×kinds` computed as
|
||
`Long` so a pathological product can't wrap `Int` back into the eligible
|
||
band and route a huge fan-out into the merge.
|
||
- **Finalizer footgun** (F2): the pooled statement's `finalize()` was renamed
|
||
`finalizeStatement()` — a no-arg `finalize()` is the JVM's `Object.finalize`,
|
||
so the GC could double-close the native handle after explicit close.
|
||
|
||
Migration cost noted (low/operational, not correctness): the synchronous
|
||
v4→v5 rebuild runs in the upgrade transaction; atomic and kill-safe (rolls
|
||
back to v4), but a very large client store pays a one-time first-open stall.
|