Skip to content

Search indexer perf: plan - #625

Merged
ajslater merged 1 commit into
v1.11-performancefrom
search-perf-plan
Apr 28, 2026
Merged

ajslater merged 1 commit into
v1.11-performancefrom
search-perf-plan

Conversation

@ajslater

Copy link
Copy Markdown
Owner

Summary

Plan for codex/librarian/scribe/search/. Single-file plan
because the surface is small (8 files, ~750 LOC).

Pure planning deliverable, no code changes.

Eight findings, three are tier-1

# Finding Severity
F1 _prefetch_related_fts_query is dead code refactor
F2 3 column-name mismatches → silent data drop correctness + perf
F3 Megaquery (10 GROUP_CONCAT aggregates + LEFT JOINs) materializes cartesian product headline perf
F4 ComicFTS.exists() per loop iteration small win
F5 time()monotonic() for elapsed reporting low
F6 clear_search_index() slow on FTS5 due to per-row token removal defer
F7 M2M .add() / .remove() doesn't bump Comic.updated_at out of scope
F8 Raw-SQL INSERT INTO codex_comicfts SELECT ... defer

Standout findings

F1 — prefetch_related + .values() dead code

Empirically verified — when a queryset chains
prefetch_related(...) with .values(), prefetches don't fire:

>>> qs = Comic.objects.prefetch_related("characters", "credits", "genres").values("pk")
>>> with CaptureQueriesContext(connection) as ctx:
...     list(qs)
>>> len(ctx.captured_queries)
1   # only the main SELECT — zero prefetch queries

Django's prefetch_related_objects walks the result cache
looking for FK descriptors, but .values() rows are plain
dicts with no descriptors. The 11 prefetch declarations in
the indexer's hot path contribute zero query work — pure
documentation. The associated comment about "1000 sqlite query
depth" is irrelevant since nothing's being prefetched.

F2 — Column-name mismatches → silent data drop

prepare.py:_COMIC_KEYS lists what the consumer reads:

_COMIC_KEYS = (..., "fts_credits", "fts_sources", "fts_story_arcs", ...)

sync.py:_M2M_FTS_RELS produces annotations under different
aliases:

_M2M_FTS_RELS = (..., "credits__person", "identifiers__source",
                      "story_arc_numbers__story_arc", ...)
# annotation alias becomes f"fts_{rel}" — i.e. fts_credits__person, etc.

The DB pays for three GROUP_CONCAT aggregations:
fts_credits__person, fts_identifiers__source,
fts_story_arc_numbers__story_arc. The Python consumer reads
fts_credits, fts_sources, fts_story_arcs. Mismatch — the
consumer's comic.get(key, "") filter silently drops them.

Two failure modes ride this single bug:

  • Perf waste: three GROUP_CONCATs run as part of the
    megaquery's cartesian product cost, results discarded.
  • Correctness regression: credits / sources /
    story_arcs columns of codex_comicfts are empty for any
    sync-built entry. Searching for a credit-person name,
    identifier source, or story-arc title silently returns no
    results. (prepare_import_fts_entry populates these
    correctly via comicbox payload keys, so import-built entries
    work — only the sync path is broken.)

Likely a regression: M2M rels were renamed to include the FK
suffix (credits → credits__person so GROUP_CONCAT walks the
right column), but the Python consumer keys weren't updated.

Fix: rename _M2M_FTS_RELS to short-name keys and use a
separate map for the GROUP_CONCAT source field. Same SQL gets
generated; the alias matches _COMIC_KEYS; the data flows
through.

F3 — Megaquery → per-M2M UNION batching

The headline perf finding. The current shape is 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 characters × 3 credit-people × 2 genres × 4
tags, the temp table before GROUP BY has 5 × 3 × 2 × 4 × ...
rows. SQLite materializes this, sorts for DISTINCT in each
GROUP_CONCAT, then GROUP BYs to collapse. Memory + I/O blow up
on richly-tagged libraries.

Apply the OPDS metadata batching pattern
(codex/views/opds/metadata.py:get_m2m_objects_by_comic):

  1. One query for the comic + simple FK columns
    (select_related for the 10 FK joins; no M2M).
  2. Ten independent queries for each M2M, each shaped
    GROUP_CONCAT(<m2m>__name) GROUP BY <m2m>__comic_id.
  3. Stitch the per-pk dicts in Python.

11 queries per batch instead of 1 megaquery. Sounds worse, but
each individual query walks one M2M's index — no cartesian
product. Cost goes from O(product of M2M counts × batch size)
to O(sum of M2M-table-rows per relation).

Suggested ordering

  1. F1 — pure cleanup. Land first, smallest diff.
  2. F2 — correctness + perf. Behavior change (sync entries
    now have credits/sources/story_arcs populated) needs a test
    pass on a populated dev DB before merge.
  3. F4 + F5 — small bundle (exists() hoist + monotonic
    clock).
  4. F3 — the headline. Largest diff, biggest payoff. Land
    after the smaller ones so a regression bisect points at the
    right commit.
  5. F6 / F7 / F8 — deferred. Open follow-up issues.

Risks flagged

  • F2 wire change: searches for credit / source / story-arc
    queries shift from "always empty" to "populated". Could be
    perceived as a regression by anyone relying on the empty-
    results behavior. Roll out with a release note.
  • F3 query reshape: if per-M2M UNION batching trips a
    SQLite planner edge case, perf could regress on small
    libraries (where the megaquery is small enough not to blow
    up). Microbench against a range of library sizes
    (1k / 10k / 100k comics).
  • F6 deferred: FTS5 token re-indexing during DELETE is
    fundamental; bulk INSERT doesn't help with token cost.
    Don't promise wins beyond what F1+F2+F3 deliver for sync.

References

🤖 Generated with Claude Code

Single-file plan because the surface is small (codex/librarian/
scribe/search/ - 8 files, ~750 LOC).

Eight findings, three are tier-1:

- F1: _prefetch_related_fts_query is dead code. Empirically
  verified that prefetch_related + .values() doesn't fire
  prefetches (Django walks the result cache for FK descriptors,
  but values() rows are dicts). 11 prefetch declarations in the
  hot path with zero query work.

- F2: Three column-name mismatches between
  _M2M_FTS_RELS annotation aliases (fts_credits__person,
  fts_identifiers__source, fts_story_arc_numbers__story_arc)
  and _COMIC_KEYS consumer keys (fts_credits, fts_sources,
  fts_story_arcs). The DB pays for three GROUP_CONCAT
  aggregations whose results are silently dropped by
  prepare_sync_fts_entry. Both perf waste AND a correctness
  bug: searching for credit-person / source / story-arc names
  on sync-built FTS entries silently returns no results.

- F3: The "megaquery" pattern (10 GROUP_CONCAT aggregations + 10
  LEFT JOIN'd FK columns + GROUP BY in one SELECT) materializes
  a cartesian product per batch. Tagged comics with rich M2M
  rows blow up the temp table. Apply the OPDS metadata batching
  pattern (per-M2M UNION queries keyed by comic_id) to avoid
  the cartesian product. Headline perf finding.

Tier 2:

- F4: ComicFTS.exists() per loop iteration in
  _get_operation_comics_query. After iteration 1 it always
  returns True; the SELECT is wasted. Hoist outside the loop.

- F5: time() -> monotonic() for elapsed_time reporting. Same
  fix as PR #623 / #624 / #625 in other librarian modules.

Tier 3 (defer):

- F6: clear_search_index() is slow on FTS5 due to per-row token
  re-indexing during DELETE. Raw `DELETE FROM codex_comicfts`
  or DROP+CREATE alternative. Defer.

- F7: M2M .add() / .remove() doesn't bump Comic.updated_at, so
  M2M-only writes never trigger FTS sync. Out of scope -
  affects importer/admin paths, not the indexer itself. Worth
  flagging.

- F8: INSERT INTO codex_comicfts SELECT ... raw-SQL path that
  eliminates the Python round-trip. Defer until F1+F2+F3
  baselines reveal whether the Python overhead is the
  bottleneck.

Suggested ordering: F1 -> F2 -> F4+F5 (small bundle) -> F3
(headline). F6/F7/F8 deferred to follow-ups.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ajslater
ajslater merged commit b631f45 into v1.11-performance Apr 28, 2026
1 check failed
ajslater added a commit that referenced this pull request Apr 28, 2026
…626)

* Search indexer perf: dead prefetch + name mismatches + small wins

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>

* Search indexer perf: per-M2M batched queries (megaquery → 12)

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>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
@ajslater
ajslater deleted the search-perf-plan branch May 2, 2026 22:39
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