Search indexer perf: dead code + name mismatches + per-M2M batching - #626
Merged
Merged
Conversation
Four findings bundled. Three are cleanups + correctness; one is
the prerequisite refactor for the headline F3 batching change in
the next commit.
F1 — drop _prefetch_related_fts_query (dead code). Empirically
verified that prefetch_related() chained to .values() doesn't
fire prefetches: Django's prefetch_related_objects walks the
result cache for FK descriptors, but values() rows are dicts
with no descriptors:
>>> qs = Comic.objects.prefetch_related("characters").values("pk")
>>> with CaptureQueriesContext(connection) as ctx:
... list(qs)
>>> len(ctx.captured_queries)
1 # only the main SELECT, zero prefetch queries
The 11 prefetch declarations (and the misleading comment about
"1000 sqlite query depth") were doing zero query work.
F2 — reconcile column-name mismatches between annotation aliases
and consumer keys. _M2M_FTS_RELS produced annotations under
fts_credits__person, fts_identifiers__source,
fts_story_arc_numbers__story_arc, but prepare.py's _COMIC_KEYS
read fts_credits, fts_sources, fts_story_arcs. The dict
comprehension's `if comic.get(key)` filter silently dropped the
mismatched data — three GROUP_CONCAT aggregations ran, results
were thrown away, and the resulting FTS columns were always
empty for any sync-built entry. Search by credit-person name /
identifier-source / story-arc title silently returned no hits.
Fix: split the rel definition into a name → path map.
_M2M_FTS_REL_MAP keeps the alias short (matches _COMIC_KEYS) but
preserves the FK-traversal path for GROUP_CONCAT. Same SQL
generated; just the alias name changes.
Behavior change: searches for credit / source / story-arc names
on sync-built entries now return real results instead of empty.
Import-built entries (via prepare_import_fts_entry) were
unaffected by this bug and stay correct.
F4 — hoist ComicFTS.exists() out of the operate loop. After
iteration 1 lands on an initially-empty FTS index, ComicFTS has
rows for the rest of the run; checking .exists() per iteration
just spends a SELECT to learn what we already know. Decide once
up-front and stash on a base_qs variable.
F5 — switch time() to monotonic() for elapsed_time reporting.
Wall-clock jumps (NTP / DST / manual adjustment) skew
time() - start_time. Same fix as PR #623 / #624 / #625 in other
librarian modules.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The headline perf finding from the plan. The previous shape ran the entire FTS row construction as one SELECT with: - 10 LEFT JOINs to FK tables - 10 LEFT JOINs to M2M through tables × 2 = 20 more - 10 GROUP_CONCAT(... DISTINCT ORDER BY ...) aggregations - GROUP BY comic.id to collapse the cartesian product For a comic with 5 chars × 3 credits × 2 genres × 4 tags × others, the intermediate temp table BEFORE GROUP BY is the product of those counts. SQLite materializes that, sorts each GROUP_CONCAT for DISTINCT, then GROUPs to collapse. Memory + I/O explode on richly-tagged libraries. Apply the OPDS metadata batching pattern (``codex/views/opds/metadata.py:get_m2m_objects_by_comic`` — also used in PR #615 for OPDS v2): 1. _build_fk_fts_rows: comic attributes + 10 simple FK joins in one SELECT. No M2M, no GROUP BY. Returns list of dicts keyed by ``id``. 2. _build_m2m_fts_dict: per-M2M, one SELECT each. 10 calls (one per M2M relation). Each walks one M2M's index independently — single LEFT JOIN, GROUP_CONCAT, GROUP BY comic_id. Returns dict[pk, comma-string]. 3. _build_universes_fts_dict: special case for the universes Concat(designation_aggregate, ",", name_aggregate). Same shape as a single M2M. 4. Stitch the per-pk dicts in Python: take each fk_row, overlay the 10 fts_<m2m> columns + fts_universes from the respective dicts, then call prepare_sync_fts_entry. Query count goes from 1 megaquery to 12 per batch (1 fk + 10 m2m + 1 universes). Sounds worse, but each individual query walks one relation's index — no cartesian product. Cost goes from O(product of M2M counts × batch_size) to O(sum of M2M-table-rows per relation), which is dramatically smaller for richly-tagged comics. Drive-by cleanup: drop _select_related_fts_query (defined but never called — dead code) and the now-unused _M2M_FTS_ANNOTATIONS constant (after the rel-map split in the previous commit, only the per-relation helpers consume the map directly). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
Implementation of
tasks/search-perf/00-plan.md(PR #625, still in flight). Two commits.Commit 1 — Dead code + name mismatches + small wins (
fc0498a2)F1 — drop
_prefetch_related_fts_query. Empirically verified thatprefetch_related()chained to.values()doesn't fire prefetches:The 11 prefetch declarations (and the misleading "1000 sqlite query
depth" comment) were doing zero query work.
F2 — reconcile column-name mismatches between annotation aliases
and consumer keys.
_M2M_FTS_RELSproduced annotations underfts_credits__person,fts_identifiers__source,fts_story_arc_numbers__story_arc, butprepare.py:_COMIC_KEYSreadfts_credits/fts_sources/fts_story_arcs. The dictcomprehension's
if comic.get(key)filter silently dropped themismatched data — three GROUP_CONCAT aggregations ran, results were
discarded, and the resulting FTS columns were always empty for any
sync-built entry.
Search by credit-person name / identifier-source / story-arc title
silently returned no hits before this fix. Behavior change: those
searches now return real results.
Fix: split the rel definition into a name → path map.
_M2M_FTS_REL_MAPkeeps the alias short (matches_COMIC_KEYS) butpreserves the FK-traversal path for GROUP_CONCAT. Same SQL generated;
just the alias name changes.
F4 — hoist
ComicFTS.exists()out of the operate loop. Afteriteration 1 lands, ComicFTS has rows for the rest of the run; the
per-iteration
.exists()SELECT was wasted.F5 —
time()→monotonic()for elapsed_time reporting.Wall-clock jumps (NTP / DST / manual adjustment) skew
time() - start_time. Same fix shape as PR #623 / #624 / #625 inother librarian modules.
Commit 2 — Per-M2M batched queries (the headline) (
1b00c9dd)The "megaquery" (1 SELECT with 10 GROUP_CONCAT aggregates + 10 LEFT
JOIN'd FK columns + GROUP BY) materialized a cartesian product per
batch. For a comic with 5 chars × 3 credits × 2 genres × 4 tags, the
intermediate temp table before GROUP BY had
5 × 3 × 2 × 4 × ...rows. SQLite sorted each GROUP_CONCAT for DISTINCT before collapsing.
Apply the OPDS metadata batching pattern:
Each per-M2M query is a single LEFT JOIN + GROUP BY comic_id —
no cartesian product across the 10 M2Ms. Cost goes from
O(product of M2M counts × batch_size)toO(sum of M2M-table-rows per relation).Three new helpers:
_build_fk_fts_rows(pks)— comic attrs + simple FK names. OneSELECT, no M2M, no GROUP BY. Returns list of dicts keyed by
id._build_m2m_fts_dict(pks, target_path)— per-M2MGROUP_CONCAT. Returns
dict[pk, comma_string]._build_universes_fts_dict(pks)— special case for universes(
Concat(designation_aggregate, ",", name_aggregate)).The batch loop builds the FK rows + 10 M2M dicts + universes dict,
then stitches per-pk dicts in Python before passing to
prepare_sync_fts_entry. Iteration order preserved viaorder_by("pk")on the FK query.
Drive-by cleanup: drop
_select_related_fts_query(defined but nevercalled — dead code) and the now-unused
_M2M_FTS_ANNOTATIONSconstant.
Test plan
make lint-pythonclean (ruff + format + basedpyright +vulture + complexipy + codespell).
make test-pythonclean (26 tests pass).make lint-frontend/make test-frontendclean.SearchIndexSyncTask(rebuild=True)and watch wall time — should drop noticeably for richly-tagged
libraries.
title / identifier-source URN returns hits. Before F2: those
searches returned zero.
credits/sources/story_arcscolumns should now containreal data instead of empty strings.
References
tasks/search-perf/00-plan.md(PR Search indexer perf: plan #625)codex/views/opds/metadata.py:get_m2m_objects_by_comic— theper-M2M UNION batching pattern this PR mirrors
time()→monotonic()fixapplied to other librarian modules
🤖 Generated with Claude Code