h-dv/code-index

Watch

Fork 0

`read_file_claim`'s diagnostics `group_concat` takes a different index from its siblings, and its comment says it does not #278

New issue

Closed

opened 2026-09-15 19:47:02 +02:00 by buildagent · 1 comment

buildagent commented

2026-09-15 19:47:02 +02:00

Member

Found in a performance review during #275 design work. Pre-existing, on a hot path, and the interesting part is that the statement's own comment asserts the opposite of what the planner does.

The claim in the source

crates/daemon/src/local_index.rs, above the statement:

ONE MORE SCALAR SUBQUERY ON THE SAME CACHED STATEMENT, and that is deliberate. … a SECOND prepare_cached/query_row round trip for the diagnostics would pay the whole per-call overhead twice to read one column off rows this statement already visits.

What the planner actually does

Three scalar subqueries, one statement, same WHERE f.path = ?1. Measured with EXPLAIN QUERY PLAN against a live .code-index/index.db:

-- the two EXISTS siblings
SEARCH f  USING COVERING INDEX sqlite_autoindex_files_1 (path=?)
SEARCH g  USING COVERING INDEX idx_plugin_generations_one_active (state=?)
SEARCH fc USING COVERING INDEX sqlite_autoindex_file_contributions_1 (file_id=?)   <-- point seek

-- the group_concat subquery
SEARCH f  USING COVERING INDEX sqlite_autoindex_files_1 (path=?)
SEARCH g  USING COVERING INDEX idx_plugin_generations_one_active (state=?)
SEARCH fc USING INDEX idx_file_contributions_generation (generation_id=?)          <-- by GENERATION

diagnostics is not a column of sqlite_autoindex_file_contributions_1, so that index stops being covering for this subquery and the planner switches to idx_file_contributions_generation. It then walks every contribution in the active generation and filters fc.file_id per row, while its two siblings do a point seek on file_id.

So it does not read a column off rows the statement already visits. It visits a different row set, through a different index, and its per-call cost scales with the size of the whole active generation rather than with this file.

Cost

Measured by the review lane over 888 invocations against a copy of the live database:

shape	time
as shipped	0.098 s
at 2 contributions per file (the #275 shape)	0.176 s (~1.85x)
forcing `INDEXED BY sqlite_autoindex_file_contributions_1`	0.001 s (~100x cheaper), and barely moves under nesting

The path matters: read_file_claim runs once per changed file for changed_symbols and review_diff, and once per path for index_coverage.

Why file it now

Two reasons beyond the number.

It is the #189 shape. That issue read as "+11.5%, the honest price of the fix" and the attribution showed 98.3% of it was ONE pre-existing unsargable clause the fix had merely handed more rows; rewriting the clause took every pinned repo BELOW baseline. #275 (region delegation) doubles the contribution count for delegating files and would hand this statement more rows. Fixing it first is what makes any later #275 cost number attributable rather than a blend of a new feature and an old defect.

A comment that misdescribes a plan is worse than no comment. This one is specific, confident, and load-bearing — it is the justification for folding the subquery into the cached statement at all. A reader optimising this path would believe it and look elsewhere.

Notes for whoever takes it

The obvious fix is INDEXED BY or restructuring so the file_id seek drives. Measure rather than assume — see below.
ANALYZE has never run on this database: SELECT COUNT(*) FROM sqlite_master WHERE name='sqlite_stat1' returns 0. Every plan above is a heuristic choice with no statistics behind it, and that is worth knowing before attributing any plan change to any code change.
There is no ORDER BY on the group_concat. Benign today — its only consumer, decode_extraction_diagnostics, sorts and dedups — but it becomes nondeterministic for any second consumer.
Adjacent, found in the same pass and probably worth the same visit: refs.influence_component_id -> extraction_components ON DELETE SET NULL has no backing index, so deleting one component full-scans the refs table (295,583 rows in this database). Unreachable in normal operation today; #275 would make a second component per file routine.

Found in a performance review during #275 design work. **Pre-existing**, on a hot path, and the interesting part is that the statement's own comment asserts the opposite of what the planner does. ## The claim in the source `crates/daemon/src/local_index.rs`, above the statement: > ONE MORE SCALAR SUBQUERY ON THE SAME CACHED STATEMENT, and that is deliberate. … a SECOND `prepare_cached`/`query_row` round trip for the diagnostics would pay the whole per-call overhead twice **to read one column off rows this statement already visits**. ## What the planner actually does Three scalar subqueries, one statement, same `WHERE f.path = ?1`. Measured with `EXPLAIN QUERY PLAN` against a live `.code-index/index.db`: ``` -- the two EXISTS siblings SEARCH f USING COVERING INDEX sqlite_autoindex_files_1 (path=?) SEARCH g USING COVERING INDEX idx_plugin_generations_one_active (state=?) SEARCH fc USING COVERING INDEX sqlite_autoindex_file_contributions_1 (file_id=?) <-- point seek -- the group_concat subquery SEARCH f USING COVERING INDEX sqlite_autoindex_files_1 (path=?) SEARCH g USING COVERING INDEX idx_plugin_generations_one_active (state=?) SEARCH fc USING INDEX idx_file_contributions_generation (generation_id=?) <-- by GENERATION ``` `diagnostics` is not a column of `sqlite_autoindex_file_contributions_1`, so that index stops being covering for this subquery and the planner switches to `idx_file_contributions_generation`. It then **walks every contribution in the active generation and filters `fc.file_id` per row**, while its two siblings do a point seek on `file_id`. So it does not read a column off rows the statement already visits. It visits a different row set, through a different index, and its per-call cost scales with the size of the whole active generation rather than with this file. ## Cost Measured by the review lane over 888 invocations against a copy of the live database: | shape | time | | :-- | :-- | | as shipped | 0.098 s | | at 2 contributions per file (the #275 shape) | 0.176 s (~1.85x) | | forcing `INDEXED BY sqlite_autoindex_file_contributions_1` | **0.001 s (~100x cheaper)**, and barely moves under nesting | The path matters: `read_file_claim` runs **once per changed file** for `changed_symbols` and `review_diff`, and once per path for `index_coverage`. ## Why file it now Two reasons beyond the number. **It is the #189 shape.** That issue read as "+11.5%, the honest price of the fix" and the attribution showed 98.3% of it was ONE pre-existing unsargable clause the fix had merely handed more rows; rewriting the clause took every pinned repo BELOW baseline. #275 (region delegation) doubles the contribution count for delegating files and would hand this statement more rows. **Fixing it first is what makes any later #275 cost number attributable** rather than a blend of a new feature and an old defect. **A comment that misdescribes a plan is worse than no comment.** This one is specific, confident, and load-bearing — it is the justification for folding the subquery into the cached statement at all. A reader optimising this path would believe it and look elsewhere. ## Notes for whoever takes it * The obvious fix is `INDEXED BY` or restructuring so the `file_id` seek drives. Measure rather than assume — see below. * **`ANALYZE` has never run on this database**: `SELECT COUNT(*) FROM sqlite_master WHERE name='sqlite_stat1'` returns `0`. Every plan above is a heuristic choice with no statistics behind it, and that is worth knowing before attributing any plan change to any code change. * There is no `ORDER BY` on the `group_concat`. Benign today — its only consumer, `decode_extraction_diagnostics`, sorts and dedups — but it becomes nondeterministic for any second consumer. * Adjacent, found in the same pass and probably worth the same visit: `refs.influence_component_id -> extraction_components ON DELETE SET NULL` has **no backing index**, so deleting one component full-scans the `refs` table (295,583 rows in this database). Unreachable in normal operation today; #275 would make a second component per file routine.

buildagent referenced this issue

2026-09-15 19:48:24 +02:00

Packages cannot see inside a multi-language file: one grammar per package, no tree-sitter injection #275

buildagent referenced this issue from a commit

2026-09-15 23:33:09 +02:00

perf(daemon): pin read_file_claim's diagnostics subquery to the file_id index (#278)

buildagent commented

2026-09-16 07:04:31 +02:00

Author

Member

Fixed in `e2ef771` — 11/11 CI green

read_file_claim's diagnostics subquery now carries INDEXED BY sqlite_autoindex_file_contributions_1 and seeks by file_id like its two EXISTS siblings.

MEASURED, 888 files, timed inside one sqlite process so no startup is counted:

shipped   0.098 s / 0.103 s        pinned   0.001 s        ~100x

The comment that asserted the opposite — that the subquery reads "one column off rows this statement already visits" — is rewritten rather than appended to, since it was specific, confident, and would have sent anyone optimising this path elsewhere.

Two repairs measured and REJECTED, recorded in the source so neither is re-attempted

ANALYZE fixes it outright — 0.002 s, planner picks the file_id seek with no hint at all. It is refused, and this tree already knew why: migrations::tests::no_query_planner_stats_are_generated pins that this schema generates no sqlite_stat1, because MEASURED on rust-analyzer those statistics make the resolver's re-heal 2.8x SLOWER (9.4 s -> 26.3 s). A local win on a warm path that is a global regression on a far hotter one. This was very nearly the fix that shipped.

Restructuring to resolve file_id to a scalar first — the repair that avoids naming a SQLite-generated index — came back at 0.282 s, WORSE THAN SHIPPED, because the scalar subquery is then re-evaluated per row of the generation scan.

The pin is gated, because this caller's failure mode is not the precedent's

local_index.rs already pins idx_refs_name and argues it is safe because a failed prepare leaves the field ABSENT — "degraded, never wrong". That argument does not transfer here. read_file_claim swallows errors with .ok().unwrap_or((0, 0, None)), so a prepare that failed because the pinned index was renamed would report NO ACTIVE CONTRIBUTION for every file: degraded AND WRONG, silently, on the surface that tells an operator whether their file is indexed at all.

crates/daemon/tests/read_file_claim_plan.rs grades three things — the pinned name resolves; every file_contributions subquery seeks by file_id; and, as a POSITIVE CONTROL, the unpinned form still takes the bad plan, so the pin test is measuring the pin rather than the planner's mood. Without that control, deleting the pin would leave a green suite.

Mutations — run, and both first verdicts were wrong

Recorded as observed rather than as predicted. Changing the pinned name reddens the SIBLING test, not the schema test, and the record now says which — crediting a test with a mutation its sibling caught proves nothing about the test under it. And a first attempt at that mutation was not the mutation at all: a sed matched a trailing quote and rewrote the two ASSERTIONS while leaving the SQL pin intact, so it went red and graded nothing.

Verified: fmt 0, clippy -D warnings 0, rustdoc -D warnings 0, cargo test --workspace 360 binaries / 3926 passed / 0 failed, and all 11 CI jobs green on e2ef771.

Still open from this issue's "notes" section

Two items deliberately NOT fixed here, because neither is reachable today and both deserve their own measurement:

refs.influence_component_id -> extraction_components ON DELETE SET NULL has no backing index, so deleting one component full-scans refs (295,583 rows in this database). Unreachable in normal operation; #275 would make a second component per file routine.
No ORDER BY on the group_concat. Benign today — its only consumer, decode_extraction_diagnostics, sorts and dedups — but nondeterministic for any second consumer.

Both are worth a look when #275 is picked up; neither blocks anything now.

## Fixed in `e2ef771` — 11/11 CI green `read_file_claim`'s diagnostics subquery now carries `INDEXED BY sqlite_autoindex_file_contributions_1` and seeks by `file_id` like its two `EXISTS` siblings. MEASURED, 888 files, timed inside one sqlite process so no startup is counted: ``` shipped 0.098 s / 0.103 s pinned 0.001 s ~100x ``` The comment that asserted the opposite — that the subquery reads "one column off rows this statement already visits" — is rewritten rather than appended to, since it was specific, confident, and would have sent anyone optimising this path elsewhere. ### Two repairs measured and REJECTED, recorded in the source so neither is re-attempted **`ANALYZE` fixes it outright** — 0.002 s, planner picks the `file_id` seek with no hint at all. It is refused, and this tree already knew why: `migrations::tests::no_query_planner_stats_are_generated` pins that this schema generates no `sqlite_stat1`, because MEASURED on rust-analyzer those statistics make the resolver's re-heal **2.8x SLOWER (9.4 s -> 26.3 s)**. A local win on a warm path that is a global regression on a far hotter one. This was very nearly the fix that shipped. **Restructuring to resolve `file_id` to a scalar first** — the repair that avoids naming a SQLite-generated index — came back at **0.282 s, WORSE THAN SHIPPED**, because the scalar subquery is then re-evaluated per row of the generation scan. ### The pin is gated, because this caller's failure mode is not the precedent's `local_index.rs` already pins `idx_refs_name` and argues it is safe because a failed prepare leaves the field ABSENT — "degraded, never wrong". **That argument does not transfer here.** `read_file_claim` swallows errors with `.ok().unwrap_or((0, 0, None))`, so a prepare that failed because the pinned index was renamed would report NO ACTIVE CONTRIBUTION for every file: degraded AND WRONG, silently, on the surface that tells an operator whether their file is indexed at all. `crates/daemon/tests/read_file_claim_plan.rs` grades three things — the pinned name resolves; every `file_contributions` subquery seeks by `file_id`; and, as a POSITIVE CONTROL, the *unpinned* form still takes the bad plan, so the pin test is measuring the pin rather than the planner's mood. Without that control, deleting the pin would leave a green suite. ### Mutations — run, and both first verdicts were wrong Recorded as observed rather than as predicted. Changing the pinned name reddens the SIBLING test, not the schema test, and the record now says which — crediting a test with a mutation its sibling caught proves nothing about the test under it. And a first attempt at that mutation was not the mutation at all: a `sed` matched a trailing quote and rewrote the two ASSERTIONS while leaving the SQL pin intact, so it went red and graded nothing. Verified: fmt 0, clippy `-D warnings` 0, rustdoc `-D warnings` 0, `cargo test --workspace` 360 binaries / 3926 passed / 0 failed, and all 11 CI jobs green on `e2ef771`. ### Still open from this issue's "notes" section Two items deliberately NOT fixed here, because neither is reachable today and both deserve their own measurement: * **`refs.influence_component_id -> extraction_components ON DELETE SET NULL` has no backing index**, so deleting one component full-scans `refs` (295,583 rows in this database). Unreachable in normal operation; **#275** would make a second component per file routine. * **No `ORDER BY` on the `group_concat`.** Benign today — its only consumer, `decode_extraction_diagnostics`, sorts and dedups — but nondeterministic for any second consumer. Both are worth a look when #275 is picked up; neither blocks anything now.

buildagent closed this issue

2026-09-16 07:04:31 +02:00

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

h-dv/code-index#278

No description provided.

Rows
Columns

read_file_claim's diagnostics group_concat takes a different index from its siblings, and its comment says it does not #278