Repository navigation
Conversation
…key checks analysis_process_steps.node_rowid references analysis_nodes(id) with foreign keys on, but the only index containing node_rowid leads with generation_id. Deleting an analysis_nodes row therefore scanned every generation's process steps, so a 1000-row GC chunk could take longer than the 30 s prune deadline on a large store and never commit. Add an index led by node_rowid. schemaSQL creates it on open, so existing stores get it without a schema version bump. The planner now also picks this index for the generation-and-node lookup; the plan test asserts that it still searches on both columns.
…imited runs A prune pass has a 30 s deadline and deleted eligible generations newest-first, so on a large backlog each pass was interrupted and the next one started on a different generation. Keep choosing the retained generations newest-first, but delete eligible ones oldest-first so a partly collected generation is finished first. When a pass ends on its own deadline, the background worker retries it with backoff from 30 s doubling to 5 minutes and keeps the prune slot; new requests coalesce into it. Other errors are not retried. DrainBackground cancels a running pass and a pending backoff. Chunk locking, per-chunk eligibility checks and the active/building exclusions are unchanged.
zzet
left a comment
zzet
left a comment
There was a problem hiding this comment.
Thanks, both root causes are right, and the scheduler tests are thorough. I mutation-checked oldest-first and the deadline retry, and each one fails its test when undone.
Please drop the old index. analysis_process_steps is WITHOUT ROWID, so analysis_process_step_node_fk(node_rowid) implicitly carries (generation_id, process_id, ordinal). On a 2.8M-row synthetic table with only the new index, both lookups are covering: SEARCH … USING COVERING INDEX (node_rowid=? AND generation_id=?) with no temp sort, and (node_rowid=?) for the FK check. Your plan test already shows the planner switching to it, so analysis_process_steps_by_node becomes unused. It's the same size (182 MB per 2.8M rows here), and every RunAnalysis insert still maintains it, which works against the store-growth goal. DROP INDEX IF EXISTS analysis_process_steps_by_node in schemaSQL would do it, plus updating the plan test's comment.
Optional: the retry loop has no no-progress stop. A chunk that can never commit within 30 s would retry every 5 min indefinitely. A cap on consecutive passes that commit nothing would bound that.
analysis_process_steps is WITHOUT ROWID, so the node_rowid index also carries the primary key and covers lookups by (generation_id, node_rowid). analysis_process_steps_by_node is no longer used but every insert still maintains it. Remove it from the schema and drop it from existing stores on open.
|
Thanks for the plan check. Done in 84e4754:
Agreed on waiting for #806. Once it merges, I'll rebase this PR onto it and resolve the |
The prune worker retries every pass that ends on its 30 s deadline. That is correct while each pass commits chunks, but a chunk that can never commit in 30 s would retry every 5 minutes indefinitely and hold the store's write lock each time. PruneAnalysisGenerations now returns the number of rows deleted by committed chunks, also when it returns an error. The worker stops after 3 consecutive deadline-limited passes that delete nothing, and logs one warning. A pass that deletes rows resets the count, so a slow prune that still makes progress keeps going. The next RunAnalysis schedules a new prune as before.
2 participants
Summary
Analysis-generation GC could not make lasting progress on a large backlog, so the store kept growing. On one v0.64.5 store, all 28 analysis generations were stale,
analysis_nodesheld about 11 million rows, theanalysis_*tables took 5.41 GB, and the daemon log had 53analysis generation prune failed ... context deadline exceededwarnings. Two causes:analysis_nodesrow checks theanalysis_process_steps.node_rowidforeign key. The only index containingnode_rowidleads withgeneration_id, so each delete scanned every generation's process steps. A 1000-row chunk could then take longer than the 30 s prune deadline, roll back and never commit.Refs #843, point 1 of this comment.
Changes
analysis_process_step_node_fk ON analysis_process_steps(node_rowid).schemaSQLcreates it on open, so existing stores get it without a schema version bump. The planner now also picks it for the generation-and-node lookup, andTestAnalysisGenerationQueryPlansUseBoundedIndexesnow asserts that this lookup still searches on both columns.keep), but delete eligible ones oldest-first, so a partly collected generation is finished first.DeadlineExceededthat is not from the per-run context, are not retried.DrainBackgroundcancels a running pass and a pending backoff.Testing
go test -race ./...)go test -race -run AnalysisGeneration ./internal/graph/store_sqlite/ ./internal/mcp/andgolangci-lint runon both packages pass. The first commit passes thestore_sqlitetests on its own.TestAnalysisGenerationGCInterruptedPruneResumesOldest,TestAnalysisGenerationPruneRetriesDeadlinesUntilCompleteandTestAnalysisGenerationPruneDrainCancelsRunAndBackofffail. Without the new index, the plan test reportsSCAN analysis_process_stepsfor the foreign-key lookup.modernc.org/sqliteprogram with the same table and index shape and 200,000 process-step rows took 1.02 s to delete 100 nodes of another generation without the index, and 1.4 ms to delete 1000 with it. It is not part of this PR.Checklist
Meta["methods"]for interfaces (if applicable)EdgeMemberOfedges to their containing type (if applicable)