[Ledger] Add GIN index for ledger item memo search#14353
Open
garyhtou wants to merge 5 commits into
Open
Conversation
Ledger::Item.search_memo (pg_search_scope, dictionary "simple") had no index behind it, so every memo search — e.g. a ledger page's `q=` filter — computed to_tsvector() on the fly across every ledger_items row in the system (not just the current ledger's), via a sequential scan. Added a functional GIN index matching the exact expression pg_search generates. Benchmarked on real production data: searching "Gary Tou" (48 real matches) went from 364ms median to 2.9ms; "Luke Oldenburg" (45 matches) went from 361ms to 3.6ms. Confirmed via EXPLAIN that Postgres now uses a Bitmap Index Scan instead of a Parallel Seq Scan. spec/models/ledger/query_spec.rb and item_spec.rb pass unchanged (85 examples) — this only changes plan/speed, not matching behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ledger::Query's $search branch ran Ledger::Item.search_memo(operand) unscoped — a full-text search across every ledger's items in the system — then intersected the result with the ledger-scoped outer relation. The GIN index added in the prior commit makes this fast regardless, but the subquery itself had no knowledge of which ledger(s) it was being run for. Stashed the ledgers/all_ledgers scope on the instance in execute() (set before apply_query runs) so the $search branch can apply the same Ledger::Mapping scoping to its own subquery that execute() applies to the final result set. Confirmed via .to_sql that the search subquery now includes the ledger_mappings scope, and end-to-end (search + paginate) on real production data dropped from 477ms to ~16-30ms warm. spec/models/ledger/query_spec.rb, item_spec.rb, and ledgers_controller_spec.rb pass unchanged (87 examples). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
All three independent reviewers of the previous commit flagged the same gap: zero test coverage for $search in query_spec.rb. Added basic $search functional coverage, plus ledger-scoping tests. Verified each new "ledger scoping" test actually discriminates old vs. new code by running it against the pre-fix version of query.rb: the cross-ledger result-set test passes on both (there was never an actual data leak — the outer scope already covered it, this was a performance fix, not a security one), so the coverage that actually pins the change down asserts on the generated SQL shape (two separate ledger_mappings subqueries: one for the search, one for the final result — vs. one on the old code). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
garyhtou
marked this pull request as ready for review
July 18, 2026 08:48
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.
Note
This PR was written by Claude Code. Gary directed the investigation and reviewed the changes, but the diagnosis, code, and benchmarks below were produced by Claude.
Summary of the problem
Searching a ledger (e.g.
/:event_id/ledger?q=...) was slow regardless of how narrow the search was.Ledger::Item.search_memo(apg_search_scope, dictionary"simple", nousing:set) had no index behind it, so every search computedto_tsvector()on the fly via a sequential scan — and that scan wasn't even scoped to the ledger being viewed, sinceLedger::Query's$searchbranch (app/models/ledger/query.rb) ran the search as its own subquery across everyledger_itemsrow in the system, only intersecting with the current ledger's items afterward.Describe your changes
Commit 1 — GIN index. Added a functional GIN index matching the exact expression pg_search generates:
Confirmed via
EXPLAINthis changes the plan from aParallel Seq Scan(withRows Removed by Filterin the hundreds of thousands) to aBitmap Index Scan.Commit 2 — scope the
$searchsubquery to the queried ledgers. The search subquery itself had no knowledge of which ledger(s) it was being run for, so it always searched system-wide before the outer query intersected the results down to the right ledger. Stashed the ledger scope on theLedger::Queryinstance (set inexecute()beforeapply_queryruns) so the$searchbranch can scope its own subquery the same wayexecute()scopes the final result set.Worth being precise about what this is and isn't: this is a performance fix, not a security fix. The pre-existing outer scoping clamp in
execute()already correctly restricted the final result set in the old code — I confirmed this empirically by reverting the change and running the new test suite against it (see commit 3): the "no data leaks across ledgers" test passes on both old and new code. The subquery being unscoped only meant Postgres did unnecessary system-wide work before the correct final intersection, not that wrong data was ever returned.Commit 3 — test coverage.
$searchhad zero existing test coverage. Added:$searchfunctional coverage$search(passes on old code too, per above — kept as general regression coverage)ledger_mappingsscope, not just the outer result set) — verified this one fails against the pre-fix code and passes against the fixBenchmarks
Searching "Gary Tou" (48 real matches — my own name, chosen so there's nothing to redact) and "Luke Oldenburg" (the term from the original slow-request report, 45 matches):
search_memo("Gary Tou")search_memo("Luke Oldenburg")Ledger::Query#execute(...).page(1).per(25))~15-120x faster depending on what's measured. 63 examples in
spec/models/ledger/query_spec.rbpass (58 pre-existing + 5 new).Out of scope
Doesn't address fuzzy/typo-tolerant search — the current search is exact-token match only (
"simple"dictionary, no stemming, nopg_trgm). That would be a separate, larger change (different extension, different index type, different match semantics) if ever wanted.Gary separately flagged that
Ledger::Query#executecould be restructured so a ledger-scoped base relation is established up front and all predicates build on top of it, rather than scoping being bolted on per-predicate as this PR does for$search. Agreed that's a good idea architecturally — it would make this whole class of "a new predicate type forgets to scope itself" bug impossible by construction rather than relying on each handler remembering to. Left for a separate PR; one thing to watch for there:$or/$norbranches (query.rb, lines building sub-relations fromLedger::Item.alldirectly) reset to a fresh base relation rather than building on the enclosing one, so that base needs to be the ledger-scopedLedger::Item.all, not a literal unscoped one, or$or/$norbranches would lose the ledger scope.