Every sec ask full-scans and temp-B-tree-sorts filing_document: no index on filing_date, plus an unconditional COUNT nothing renders #366

Activity

1. sroussey commented on Sep 15, 2026

   

   sroussey

   on Sep 15, 2026 – with Claude

   ContributorAuthor

   More actions

   Both fixes landed on claude/admiring-lovelace-59uare — 4d840aeb. No PR opened.

   1. The index

   src/config/storageRegistry.ts:239-249 — one more entry beside the two already there:

   indexes: [
     ["form", "converter_version", "is_primary"],
     ["converted_at"],
     ["filing_date", "accession_number"],
   ],

   ASC/ASC rather than the DESC/DESC of your probe: defineStorage's indexes cannot express per-column direction, and it does not need to — the ORDER BY reverses both columns, so SQLite walks the same index backwards and the temp B-tree goes away either way. That is the property the column order has to match, and it is what the test below asserts rather than assuming.

   2. The count

   countAlreadyIndexed is now asked for rather than assumed. IndexFilingSectionsTaskInput grows countSkipped, defaulting to counting so a caller has to have decided not to, and IndexFilingSectionsTaskOutput.skipped becomes number | undefined — absent when a run did not count, not zero, because zero is an answer (the index holds nothing in scope) and not what "nobody asked" means. src/cli/groups/ask.ts:115-121 passes countSkipped: false on the pre-index; the sec index renderer, which is the one caller that prints the number, is unchanged in behaviour.

   Worth stating why the index does not help this one: it is an unbounded aggregate over the join, so it visits every matching row no matter what is indexed. Measured at 139.1 ms before the index and 140.2 ms after — the only way to make it cheaper is not to run it.

   Measured

   Rerun from scratch rather than quoting the numbers above. Real SQLite file (WAL, temp_store = MEMORY), the schema built by SqliteTabularStorage from FilingDocumentSchema with the real index set, 300,000 rows, a tenth already in kb_document, LIMIT 26, ANALYZE run, mean of 5 executions after a warm-up. Node 24.20.0 / Bun 1.4.2.

   BEFORE — indexes sqlite_autoindex_filing_document_1, filing_document_converted_at, filing_document_form_converter_version_is_primary:

   EXPLAIN QUERY PLAN (selection):
     SCAN d
     SEARCH k USING COVERING INDEX sqlite_autoindex_kb_document_1 (doc_id=?) LEFT-JOIN
     USE TEMP B-TREE FOR ORDER BY
     → 139.7 ms
   
   EXPLAIN QUERY PLAN (countAlreadyIndexed):
     SCAN d USING COVERING INDEX sqlite_autoindex_filing_document_1
     SEARCH k USING COVERING INDEX sqlite_autoindex_kb_document_1 (doc_id=?)
     → 139.1 ms
   

   AFTER — the same, plus filing_document_filing_date_accession_number:

   EXPLAIN QUERY PLAN (selection):
     SCAN d USING INDEX filing_document_filing_date_accession_number
     SEARCH k USING COVERING INDEX sqlite_autoindex_kb_document_1 (doc_id=?) LEFT-JOIN
     → 0.2 ms
   
   EXPLAIN QUERY PLAN (countAlreadyIndexed):
     SCAN d USING COVERING INDEX sqlite_autoindex_filing_document_1
     SEARCH k USING COVERING INDEX sqlite_autoindex_kb_document_1 (doc_id=?)
     → 140.2 ms          ← not run at all on `sec ask` any more
   

   The plans are exactly the ones in the report. My absolute numbers are lower (139.7 ms vs 415 ms for the unindexed selection) — different machine and a different row mix — but the shape is identical: SCAN d + USE TEMP B-TREE FOR ORDER BY becomes SCAN d USING INDEX, and O(N) becomes O(limit).

   Per sec ask, on that corpus: ~279 ms → ~0.2 ms. The selection is ~700× cheaper and the second scan is gone.

   What an operator with an existing database has to run

   sec db setup

   That is all — setupDatabase() emits every declared index as CREATE INDEX IF NOT EXISTS on every run, not only on a fresh database, so a newly declared index appears on the next setup and no rows move. (sec setup works too; it calls the same path.) This is the opposite of a new column, which CREATE TABLE IF NOT EXISTS cannot add and which needs the catch-up pass — so I verified the path rather than assuming it, against a database created at the old shape and carrying a row:

   before `db setup`: [ filing_document_converted_at,
                        filing_document_form_converter_version_is_primary,
                        sqlite_autoindex_filing_document_1 ]
   after  `db setup`: [ filing_document_converted_at,
                        filing_document_filing_date_accession_number,
                        filing_document_form_converter_version_is_primary,
                        sqlite_autoindex_filing_document_1 ]
   row survived: 1
   

   docs/fetch-and-storage.md §5 now says this, beside the column passes it is easily confused with. On a large filing_document the CREATE INDEX itself is the cost — it reads the table once under a write lock.

   Tests

   • src/task/kb/selectDocumentsToIndex.sqlite.test.ts — reads the newest filings from an index rather than sorting the table. It spies db.prepare, captures the statement and its bound params that selectDocumentsToIndex actually prepares (proxying .all), then runs EXPLAIN QUERY PLAN on that exact statement: asserts the plan names filing_document_filing_date_accession_number and that no step is USE TEMP B-TREE FOR ORDER BY. Planned from the real statement rather than looked up in sqlite_master on purpose — an index whose column order does not match the ORDER BY exists and removes nothing. Confirmed to fail without the registry entry, reproducing your "before" plan in a unit test:

     AssertionError: expected 'SCAN d\nSEARCH k USING COVERING INDEX…' to contain
                     'filing_document_filing_date_accession_number'
     + SCAN d
     + SEARCH k USING COVERING INDEX sqlite_autoindex_kb_document_1 (doc_id=?) LEFT-JOIN
     + USE TEMP B-TREE FOR ORDER BY
     

   • src/task/kb/IndexFilingSectionsTask.test.ts — does not count what is already indexed for a caller that will not show it: three filings, all indexed, countSkipped: false; asserts no COUNT(*) statement is prepared and that skipped comes back undefined.

   Not addressed

   The two items under "Smaller, same file" are untouched and still stand — countAlreadyIndexed omitting the section_count > 0 clause the selection applies, and selectByScan returning repository order where the JSDoc promises newest-first. Both are correctness divergences rather than cost, and neither is in this change.

   Also untouched: "consider whether ask's implicit index needs a selection at all when kb_chunk is non-empty and the scope is unchanged." With the index in place the selection is O(limit), so the remaining prize there is small.

   Verification

   command	exit	time
   bun run format-check	0	—
   bun run lint	0	—
   bun run typecheck	0	1.7 s
   bun run build	0	—
   bunx vitest run src/task/kb src/config src/cli src/commands	0	—
   bunx vitest run (full suite)	0	132 s — 1496 passed, 21 skipped, 151 files

   No timeouts and no load-sensitive failures in the verify tier on this run.

   ---

   Generated by Claude Code
2. sroussey

   closed this as completedon Sep 21, 2026
3. sroussey commented on Sep 21, 2026

   

   sroussey

   on Sep 21, 2026 – with Claude

   ContributorAuthor

   More actions

   Both halves fixed by 31d72bdd ("perf(kb): page kb_chunk by key rather than by offset (#373)"), whose second commit is "index filing_document for the index selection, and skip the count nobody reads". Verified at d544bc64 by execution.

   1. The index is declared, and it reaches a real database. src/config/storageRegistry.ts now declares three indexes on filing_document, the third being ["filing_date", "accession_number"] in exactly the ORDER BY order. On a scratch database built by the shipped binary:

   $ sec db setup
   $ SELECT name, sql FROM sqlite_master WHERE type='index' AND tbl_name='filing_document'
   filing_document_converted_at                  | CREATE INDEX … (`converted_at`)
   filing_document_filing_date_accession_number  | CREATE INDEX … (`filing_date`, `accession_number`)
   filing_document_form_converter_version_is_primary | CREATE INDEX … (`form`, `converter_version`, `is_primary`)
   

   An existing deployment picks it up from db setup (every declared index is emitted as CREATE INDEX IF NOT EXISTS on every run), and docs/fetch-and-storage.md gained a section saying so and distinguishing it from a new column.

   2. The temp B-tree is gone, and the win is large wherever anything is still unindexed. The exact statement selectDocumentsToIndex prepares, on 300,000 filing_document rows, median of 3 after a warm-up, swept over how much of the corpus is already in the knowledge base:

   already indexed   no new index   WITH new index   rows returned
               0%        98.8 ms          0.1 ms        26
              50%       216.8 ms          0.1 ms        26
              90%       346.8 ms          1.7 ms        26
              99%       238.4 ms         21.1 ms        26
   

   Plan without: SCAN d | SEARCH k … LEFT-JOIN | USE TEMP B-TREE FOR ORDER BY.
   Plan with: SCAN d USING INDEX filing_document_filing_date_accession_number | SEARCH k … LEFT-JOIN — no temp B-tree, O(limit) rather than O(N).

   selectDocumentsToIndex.sqlite.test.ts pins this the right way: it takes EXPLAIN QUERY PLAN of the statement the selection actually prepares (intercepted through db.prepare), not the presence of a row in sqlite_master — "an index whose column order does not match the ORDER BY exists and removes nothing."

   3. The unconditional COUNT is gone from the path that never renders it. IndexFilingSectionsTask gained a countSkipped input defaulting to counting, sec ask's implicit pre-index passes countSkipped: false, and skipped is now number | undefined — absent rather than zero, "because zero says the index holds nothing in scope." IndexFilingSectionsTask.test.ts spies db.prepare and asserts no COUNT(*) statement is prepared. Measured cost of the removed query on the same 300k corpus: ~104–112 ms per question, unaffected by any index (an unbounded aggregate visits every matching row).

   One residual, filed separately rather than left here. In the fully-indexed steady state — nothing left to index, which is what sec ask sees from the second question onward — the selection still traverses the whole table (342 ms at 300k rows on a realistically ordered corpus), and the new index is a wash to worse in exactly that regime: 1.2× slower where physical row order tracks filing_date (418 ms vs 342 ms), and up to 11× slower (2,646 ms vs 240 ms) where it does not, because walking the index in date order turns the kb_document probes into random seeks where the table scan issued them in accession order. The index is still the right call — the regime it helps is the one with work to do — but the steady state is unfixed and now slightly worse.

   Closing as completed; the residual is its own issue.

   ---

   Generated by Claude Code