Skip to content

Add V215: six indexes on the agent investigation tables - #6637

Merged
erikdarlingdata merged 4 commits into
devfrom
fix/agent-table-indexes-v215
Oct 23, 2026
Merged

erikdarlingdata merged 4 commits into
devfrom
fix/agent-table-indexes-v215

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 23, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Adds migration rung V215 with six indexes on the investigation ("agent") tables. A restored store copy loaded with one synthetic year of requests (73,000) at a production store's measured rate timed every hot read, in two runs. Most reads stay under 1 ms. These did not at a year. Each index was tested inside a rolled-back transaction.

Read Before (wall ms) After Index
Follow-up module read (ProcedureStatsModuleDates.WatchedSql, about 60k calls a day) 127 5.5 1 + 2
Due follow-ups read (DueFollowUpsSql) 35 0.7 3 + 2
PendingFollowUpsSql 25 5 2
MCP list_investigation_requests, reports 30-day read 25-53 0.13-0.3 4
Brief earlier-report and prior-links lookups, live-practice hider 12-24 0.01-0.25 5
Retention delete of requests (fk_investigation_reports_family check) 310 for 8 rows 0.5 6

The resolve check is fixed separately in #6633 (no rung).

What changed

  • PgMigrations.cs: V215 agent-table-indexes, plain CREATE INDEX IF NOT EXISTS. The tables hold hundreds of rows on production stores today, so each build is instant. Named ix_investigation_requests_reported_condition, _follow_up_of, _follow_up_at, _opened_at, _practice_of and ix_investigation_reports_family_of.
  • StorageVersion.SchemaVersion is 215.
  • Viewer probe: a pg_indexes sentinel on the reports index, a newest-first hasAgentTableIndexes arm returning 215, and the ladder entry with its drop SQL in RungRewindArtifacts. MigrationProbeLadderTests covers the mapping only (synthetic booleans). The live AgPrimaryReplicaRungTests path runs the real probe SQL and proves a V215 store probes 215. No literal 214 pins existed in the tests.
  • V184's InvestigationTables.CreateSql is not edited. A fresh store runs every rung and gets the indexes.
  • Provisioning: no change. An index needs no GRANT, and the roles files do not mirror rung DDL.

Predicate check against origin/dev

Each read's WHERE clause implies its index predicate, so the planner can use it. No predicate needed changing.

  • 1: WatchedSql has state = 'reported' and follow_up_condition IS NOT NULL.
  • 2: every read tests f.follow_up_of = r.request_id, which implies NOT NULL.
  • 3: DueFollowUpsSql has state = 'reported' and follow_up_at IS NOT NULL.
  • 4: ReportsRequestsSql and ListSql order and filter on opened_at.
  • 5: LivePracticeRequestsSql has practice_of IS NOT NULL, and the lookups join on practice_of =.
  • 6: the foreign key check on family_of is an equality.

Tests

There is an established plan-test pattern (PgStatementTextDueIndexLiveTests), so I added one for the hottest read.

Test Revert proof Status
AgentTableIndexesV215LiveTests.TheFollowUpModuleRead_UsesTheConditionIndex_AndNotOnceItIsDropped: seeds 73,000 reported requests, EXPLAINs WatchedSql, expects the condition index, then drops it and expects the plan to lose it Remove index 1 from V215 and the first assert fails. The drop half only proves the EXPLAIN is re-planned. A follow-up PR asserts the Seq Scan fallback, so that half fails on a defect too waits for CI
AgPrimaryReplicaRungTests (existing, live probe) Leave out the sentinel line or the rewind entry and it fails waits for CI
MigrationProbeLadderTests and the ladder density tests Cover the mapping and rung density only. They do not run the probe SQL waits for CI

Darling.Tests builds on macOS but can't run there. Locally I applied the exact V215 SQL twice to a scratch Timescale container (idempotent, six indexes). I also ran WatchedSql on 73,000 synthetic rows: with the index it plans an Index Scan on it, and without it a Seq Scan. I stopped the container afterwards.

Considered, not added

  • Lite (DuckDB) gets no indexes, for the reason its schema already records (Lite/Database/Schema.cs:416-417). DuckDB runs an UPDATE of an indexed column as a delete and an insert, and a second update of the same row in one transaction trips over it. follow_up_at and follow_up_condition are columns an UPDATE writes. Lite's investigation_requests also has no practice_of and no follow_up_condition column, so indexes 1 and 5 have nothing to index there. CONTRIBUTING's parity rule covers a rung plus a reader, and V215 adds no reader.
  • No test for the fk_investigation_reports_family check using index 6. No trigger: the index leads on family_of, the FK column, so an equality probe can use it. Cost is a live PG test I can't run here. Verify in the field.
  • No lock_timeout on V215. It follows V207, a partial index on config.investigation_requests with no lock_timeout. The rungs that set one (V152, V153, V154, V173 and V175) lock tables a background refresh policy can hold for about 264 s (PgMigrations.cs:83-97). These are plain config tables that no refresh policy locks. Builds are milliseconds on tables this narrow, and the census pin keeps the 5x lock wait.
  • No other indexes. Only these six have a measured trigger.
  • Rung order: Darling collects Query Store hints: get_query_store_hints lists them and Change Watch sees one set or cleared (#6581) #6619 and Agents can recompile a module and create or drop a plan guide through Level 3 server actions, with Undo (#6569) #6635 also say V215. Whichever merges after this one renumbers, as the skill's rule says.

CHANGELOG entry

  • Agent investigation tables: six new indexes (schema V215) keep follow-up, request list, brief and retention reads fast as a year of requests builds up. The follow-up module read drops from 127 ms to 5.5 ms and the retention delete of requests from 310 ms to 0.5 ms in a one-year synthetic load.

CI fix

Two tests failed in the Build run.

  • MigrationDataMovingRungCensusPins.TheDataMovingRungSet_EqualsTheDeclaredCensus: V215's index builds read rows V184 created, so the scan counts it. I declared V215 in s_declared with SetsTheFloor: false. Both tables are narrow control-plane tables (hundreds of rows on a store today, 73,000 in the year-scale test), so each build takes milliseconds. MigrationLockWaitTimeoutSeconds stays at 5x. I did not hide the rung from the scan.
  • ViewerKnownPatternEditorLivePostgresTests.OnAStoreBelowV214...: dropping the V214 table left V215's indexes, so the probe answered 215. The test now also calls RungRewindArtifacts.DropSentinelsAboveAsync(connection, 214, ct), as the other below-Vn tests do. No other test drops a V214 sentinel and then probes (the brief reader test does not probe).
  • Darling.Tests builds with 0 errors. The two tests wait for CI (no local PostgreSQL run).

Review round

  • Lite rationale: body was wrong. Rewritten in Considered, not added.
  • MigrationProbeLadderTests overclaim: body was wrong. Corrected in What changed and Tests.
  • Riskiest line 3 (FK index uncovered): no trigger. Listed under Considered, not added.
  • Lock on largest store: no trigger. Listed under Considered, not added.
  • V215 collision: dev SchemaVersion is still 214 at this check. The later merger renumbers.
  • Unverified timing and container claims: not re-measurable here. Left as stated.
  • No code change.

Review round 2 (review at f9b923e)

  • The lock_timeout precedent was inverted: V153 sets one. The body now cites V207 and says why these tables need none.
  • The Lite rationale is replaced with the reason Schema.cs:416-417 already records.
  • The drop half of the live test proves only that the EXPLAIN is re-planned. The reviewer's Seq Scan assertion goes in a small test-only follow-up PR, so this rung does not hold the role-file slot for another CI run.
  • No code change in this round.

A one-year synthetic load on a restored store copy showed follow-up, request list,
brief and retention reads slowing as requests build up. Six partial and plain
indexes bring each back under 10 ms.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 23, 2026 14:01
claude[bot]
claude Bot previously requested changes Oct 23, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

The rung is clean. One thing needs fixing, and it is a sentence in the body, not a line of code.

Claims

  • VERIFIED - V215 agent-table-indexes adds the six named indexes - PgMigrations.cs:2305-2317, registered :2708.
  • VERIFIED - SchemaVersion is 215 - StorageVersion.cs:19, pinned symbolically at AgentLevel2StoreTests.cs:50.
  • VERIFIED - Probe sentinel plus newest-first arm, all four parts - ViewerDataService.cs:1337, :1587-1590, :1381, :1359. 191 top-level probe expressions (765-1337) against 191 bool has parameters, so GetBoolean(190) cannot run off the end. V214 correctly demoted at :1591.
  • VERIFIED - Ladder entry and drop SQL present - RungRewindArtifacts.cs:508-512; DROP INDEX IF EXISTS applies per name, safe below V184 where the table is absent.
  • VERIFIED - No test pinned 214 as the top; the one literal 214 is V214's own feature gate, which already has a 215 case - ViewerKnownPatternEditorTests.cs:100-101.
  • VERIFIED - V184's InvestigationTables.CreateSql untouched - not among the five changed files.
  • VERIFIED - A fresh store runs every rung - PgMigrations.cs:9595-9600, the path the new test takes via OpenStoreAsync.
  • VERIFIED - No provisioning change needed; the six names appear nowhere else, and the README rollback list is explicitly non-exhaustive - Darling/README.md:295.
  • VERIFIED - Predicates 1 and 2 - ProcedureStatsModuleDates.cs:44,46,48 and InvestigationRequestStore.cs:185,229 (a strict equality implies NOT NULL).
  • VERIFIED - Predicates 3 and 4 - InvestigationRequestStore.cs:225,227,230 and :419, :485-486.
  • VERIFIED - Predicates 5 and 6 - InvestigationRequestStore.cs:523 and PgInvestigationBriefSourceReader.cs:109,150,251; family_of is the only one of five FKs into the requests table whose column lacked a leading-column index, the other four leading on request_id (InvestigationTables.cs:134,156, PgMigrations.cs:1506-1507,1617). That is what makes the sixth index the right fix for the 310 ms.
  • VERIFIED - The new test follows the established pattern and its seed satisfies every NOT NULL column and both CHECKs - AgentTableIndexesV215LiveTests.cs:32-78 against PgStatementTextDueIndexLiveTests.cs:28-60 and InvestigationTables.cs:78-113.
  • UNVERIFIED - The before/after wall-millisecond table - needs the restored store copy and the synthetic year.
  • UNVERIFIED - "Each index was tested inside a rolled-back transaction" - a property of the session, not the tree.
  • UNVERIFIED - "Remove index 1 and the first assert fails" - needs a live DARLING_TEST_PG and a mutation run.
  • UNVERIFIED - The local twice-applied scratch container and its plan result - needs the container.
  • UNVERIFIED - "#6619 and #6635 also say V215" - another PR; only 215's density here is checkable (MigrationLadderPins.cs:135-148).
  • UNVERIFIED - "The resolve check is fixed separately in #6633" - another PR.
  • REFUTED - "the investigation tables are a Darling feature". Lite defines investigation_requests and investigation_reports (Lite/Database/Schema.cs:429,491), registers both in GetAllTableStatements() (:591,593), ships Lite/Mcp/McpInvestigationTools.cs and Lite/Services/InvestigationSweepService.cs, and the changelog records "Lite: Level 1 agent investigations" (CHANGELOG.md:45). Lite carries the counterpart reads too: the same follow_up_of anti-join at DuckDbInvestigationStore.cs:153 and opened_at filters at DuckDbInvestigationStore.Evidence.cs:69,81.

Riskiest lines

  1. ViewerDataService.cs:1337 - a wrong sentinel name, position or ordinal makes a current store probe 214 and the gate at MainWindow.xaml.cs:409 refuse it permanently - covered by AgPrimaryReplicaRungTests.AFreshStore_AndOneUpgradedFromThePriorRung_StoreAndReadBackThePrimary_WithTheColumnLast, which runs the real probe SQL on a migrated store and asserts SchemaVersion (:54,189-205); its :63-66 half also fails if the rewind entry misses the sentinel.
  2. PgMigrations.cs:2307 - a predicate the read's WHERE does not imply builds an index the planner can never use, costing writes for nothing with every assertion green - covered by AgentTableIndexesV215LiveTests.TheFollowUpModuleRead_UsesTheConditionIndex_AndNotOnceItIsDropped.
  3. PgMigrations.cs:2316 - the reports index. Its existence is covered via the probe, but nothing exercises the fk_investigation_reports_family ON DELETE SET NULL check using it, so a predicate the RI query could not use leaves the 310 ms in place, green - UNCOVERED.

Traps near this diff

  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" - does not apply - no reader, tool, alert or description here, and an index adds no state; the clause is scoped to "State added to one store" (CONTRIBUTING.md:309-313), and Lite's table has neither practice_of nor follow_up_condition and declares no index (Lite/Database/Schema.cs:429-465).
  • "Ruling (#5833): tests are load-bearing. ... never answer a trap with a test that reads source text or asserts how code is spelled" - does not apply - the new test asserts a real plan and adds no census (AgentTableIndexesV215LiveTests.cs:50-78).
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." - does not apply - the names are DDL once (PgMigrations.cs:2306-2317) and catalog literals twice, the established sentinel shape (ViewerDataService.cs:770,793); a stale literal fails the live probe test rather than hiding behind a pin.
  • "A test that reads the clock twice measures the CI scheduler ... One anchor owns both ends of an interval." (#3676) - does not apply - no duration is asserted and every seeded timestamp comes from one now() in a single statement (AgentTableIndexesV215LiveTests.cs:60-69).
  • ".gitattributes says text=auto eol=crlf ... A diff that touches every line of a file is a line-ending conversion until proven otherwise." - does not apply - 127 additions, 5 deletions, five files; the long ViewerDataService.cs lines are genuinely one-call-per-line.

Divergence (blind read vs PR body)

The Lite rationale. "The investigation tables are a Darling feature" is contradicted by Lite's own schema, store, sweep service and MCP tools, and Lite carries the very reads these indexes serve. The conclusion - no Lite counterpart - is still right, for a different reason: DuckDB has no partial indexes, its ART index serves equality and uniqueness rather than the range scans and ORDER BY opened_at DESC these six are for, Lite declares no index on any investigation table, and a single-seat store never reaches the year of requests that made these reads slow. Please state that instead: "it's a Darling feature" would equally wave away a Lite twin for a reader, which the two-store rule calls a finding.

Smaller: the body credits MigrationProbeLadderTests with covering the probe. It covers the mapping only, by reflection over synthetic booleans (MigrationProbeLadderTests.cs:40-48), and never executes StoreSchemaProbeSql - a missing sentinel line or wrong GetBoolean index passes it. The live AgPrimaryReplicaRungTests path is what catches that, so a green unit run is not the probe being proven.

Reviewer checklist

  • Run with DARLING_TEST_PG set: AgentTableIndexesV215LiveTests and AgPrimaryReplicaRungTests, the only test proving a V215 store probes 215 rather than 214.
  • On real data, confirm the fk_investigation_reports_family check plans through ix_investigation_reports_family_of; it is a generic SPI plan, so verify in the field (riskiest line 3, uncovered).
  • Confirm the builds take no noticeable lock on the largest store: CREATE INDEX takes ShareLock and the rung carries no SET LOCAL lock_timeout (matching V153, but resting on the unverifiable "hundreds of rows").
  • Settle the V215 collision with #6619 and #6635, and confirm the loser renumbers rung and StorageVersion together.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

Follow-up review (range 7e94df73c..cc3027d8f)

This range contains exactly one new commit after the previously-reviewed 7e94df73c (the V215 migration): cc3027d8f, "Merge remote-tracking branch 'origin/dev' into HEAD".

Checked whether that merge is purely mechanical or carries any author-authored conflict resolution:

  • The merge's two parents are 7e94df73c (this PR's V215 commit) and 0a6121e1c (dev tip, six unrelated commits: 0a6121e1c, 3a32c4597, 6eed85912, 9401603a8, a0973aa4f, 4d4298deb).
  • git diff 7e94df73c cc3027d8f is non-empty only because those six dev commits aren't yet in 7e94df73c's history — i.e. it's the normal union of two branches, not new work.
  • The files V215 touches (PgMigrations.cs, StorageVersion.cs, RungRewindArtifacts.cs, ViewerDataService.cs) are untouched by any of the six dev commits (git diff from their common merge-base to dev tip on those paths is empty), so there's no overlap and nothing for the merge to resolve. git diff 7e94df73c cc3027d8f on those same four files is also empty, confirming the merge carried V215 through unchanged.

So this merge brings in the base branch's work and nothing else — no new author change, and no interaction with the already-reviewed V215 commit. Per the follow-up scope, that work isn't re-reviewed here, and the prior review's open finding (the "investigation tables are a Darling feature" rationale in the PR body) is unaffected by this commit, so it isn't repeated in this pass.

Nothing in this range requires changes.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

No new claims to grade: the only new commit in this range is cc3027d8f, a merge of origin/dev with an empty commit message body. The PR body's claims describe the earlier 7e94df73c commit, already graded in the prior full-pass review, so they are not re-graded here.

Riskiest lines

No lines changed by this range's author content. The merge is confirmed mechanical: git diff 7e94df73c cc3027d8f on the four files V215 touches (PgMigrations.cs, StorageVersion.cs, RungRewindArtifacts.cs, ViewerDataService.cs) is empty, and the six dev commits brought in by the merge don't touch those files at all (verified via their common merge-base ada395ecd).

Traps near this diff

None — there is no new authored diff in this range to hold against the traps file.

Divergence (blind read vs PR body)

None — the merge commit carries no message body and makes no claims to compare against the diff.

Reviewer checklist

  • The prior review's open item (PR body's "the investigation tables are a Darling feature" rationale, refuted by Lite's own schema/store/MCP tools) is still unaddressed in the current PR body text — worth fixing before merge, but it predates this range and isn't a defect introduced by cc3027d8f.

@erikdarlingdata
erikdarlingdata marked this pull request as draft October 23, 2026 16:58
@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 23, 2026 20:41

Claude posting for Erik Darling. Answered at head 4064092. The Lite rationale and the probe-test claim were wrong in the body and are fixed. The FK-index test and lock_timeout points have no concrete trigger and are listed under Considered, not added. No code change was needed.

/// (<c>InvestigationTables.CreateSql</c>) is not edited, and a fresh store runs every rung, so it gets these too. Additive: an older build
/// never reads the indexes, and no GRANT is needed for an index.</para>
/// </summary>
private const string V215Sql = @"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body's lock_timeout precedent is inverted. "Considered, not added" says: "No lock_timeout on V215. It matches V153." V153 does set one — PgMigrations.cs:206:

private static readonly string V153Sql = @"
SET LOCAL lock_timeout = '" + (MigrationCommandTimeoutSeconds - 20) + @"s';
SET LOCAL max_parallel_maintenance_workers = 2;
CREATE INDEX IF NOT EXISTS idx_query_store_interval_latest_first_exec
ON collect.query_store_interval_latest (first_execution_time);";

So does V154 (:219), V152 (:100), V173 (:932) and V175 (:1008) — and :83-97 records why: those rungs take a lock a background refresh policy can hold for ~264 s, so the rung needs the server's own retryable 55P03 to fire before the client-side command timeout cancels the statement.

Omitting it here is the right call, but the precedent is the other set: the index-only rungs on non-hypertable tables set none — V104 (collect.pg_deadlocks), V113 (collect.plan_force_actions) and V207 (config.investigation_requests itself, ChangeWatchStore.cs:177). V207 is the exact shape: a partial index on this very table, no lock_timeout. Citing V153 instead tells the next reader that a rung racing a 264-second refresh doesn't need one, which is the failure :83-97 was written to prevent.

One-word fix in the body — V207, not V153. No code change.


await ExecAsync(connection, "DROP INDEX config." + ConditionIndex, ct);
var without = await ExplainAsync(connection, ct);
Assert.DoesNotContain(ConditionIndex, without, StringComparison.Ordinal);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The drop half proves the harness, not the plan. Once DROP INDEX has run, no plan PostgreSQL can produce is able to name a dropped index, so this assertion fails on no defect in V215 — only on an ExplainAsync that returned a stale or constant string. That is worth something (it shows the EXPLAIN really is re-planned against the catalog), but it is not the control the class doc sells it as: "the read must go through V215's partial index on the condition, and must not once that index is gone."

The assertion that would fail on a defect is the one you already measured by hand — the body says "with the index it plans an Index Scan on it, and without it a Seq Scan." Assert the fallback:

Suggested change
Assert.DoesNotContain(ConditionIndex, without, StringComparison.Ordinal);
Assert.DoesNotContain(ConditionIndex, without, StringComparison.Ordinal);
/* The control that fails on something: with the index gone the only path left is the whole table, so a plan that
still finds a cheap one means the seed never made the index load-bearing and the first assertion was free. */
Assert.Contains("Seq Scan", without, StringComparison.Ordinal);

That pins what the 127 ms → 5.5 ms measurement actually rests on: at a year of requests the read has no alternative to a full scan. Without it, a seed that quietly stops populating follow_up_condition (one row in 200, line 62) leaves both assertions green while the first has stopped discriminating.

Non-blocking.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown

The Lite rationale lands on the right answer by the wrong road

"Considered, not added" says: "Lite has the same tables and reads, but DuckDB has no partial indexes, and its ART index serves equality rather than the range scans and ORDER BY opened_at DESC these six are for." Three pieces of that do not survive a read of Lite/:

  • Lite does not have the same tables. investigation_requests there (Lite/Database/Schema.cs:430-465) carries no practice_of and no follow_up_condition, and DuckDbInvestigationStore.cs:146 says so outright: "Lite has no practice requests, so there is no practice_of filter." Indexes 1 and 5 have no Lite counterpart to decline — there is no column to index.
  • Three of the six are equality, not range. Indexes 2 (follow_up_of), 5 (practice_of) and 6 (family_of) are plain equality probes, and Lite has follow_up_of (Schema.cs:461) and family_of (Schema.cs:508). The ART-serves-equality argument argues for those two on Lite, not against them.
  • The real reason is already recorded, and it is stronger. Schema.cs:416-417, in the v69 step's own note on what shapes these tables: "No secondary index, and none on a column an UPDATE writes: DuckDB runs an UPDATE of an indexed column as a delete and an insert, which a second update of the same row in one transaction trips over." follow_up_at and follow_up_condition are exactly columns an UPDATE writes (SetFollowUpSql, SetFollowUpConditionSql), so indexing them on Lite would not be a missed optimisation — it would be a correctness hazard the schema already refuses.

The conclusion (no Lite change) is right, and CONTRIBUTING's parity rule does not reach this rung either: CONTRIBUTING.md:310 scopes it to "a Darling migration rung plus a reader for it", and V215 adds no reader and no state. Worth swapping the rationale for the one the tree already holds, so the next rung that wants a Lite index reads the hazard rather than the ART argument.

Non-blocking.

Things that held up, for the record

  • Every partial predicate is implied by its read's WHERE. 1 → ProcedureStatsModuleDates.cs:44-46 (state = 'reported', follow_up_condition IS NOT NULL); 2 → the equality f.follow_up_of = r.request_id in InvestigationRequestStore.cs:185 and :229 and ProcedureStatsModuleDates.cs:48; 3 → InvestigationRequestStore.cs:225-227; 5 → LivePracticeRequestsSql at InvestigationRequestStore.cs:523 and InvestigationPracticeStore.cs:171,193; 6 → the ON DELETE SET NULL trigger's WHERE $1 = family_of (PgMigrations.cs:1591-1592). No predicate is unreachable, and index 5 is practice_of IS NOT NULL rather than the IS NULL that most reads of that column use — which is correct, because the hider it serves is the IS NOT NULL side.
  • The probe's two halves are in step. 191 top-level expressions in StoreSchemaProbeSql, 191 parameters on MapProbedSchemaVersion, GetBoolean(0..190). The live AgPrimaryReplicaRungTests (:54, :72) runs the real probe SQL against a fully migrated store and asserts StorageVersion.SchemaVersion, so a miscount is caught there rather than in the field.
  • All six index names match between rung and rewind, each appearing exactly twice across Darling/ (plus the sentinel's third use in the probe and the condition index's in the new test). A rewind leaves no sentinel behind.
  • The rewind runs newest-first (RungRewindArtifacts.cs:521), so V215's DROP INDEX fires while config.investigation_requests still exists on every chain that later drops the table at V184.

claude[bot]
claude Bot previously requested changes Oct 23, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — six indexes as rung V215 — PgMigrations.cs:2305-2317, Migration(215, "agent-table-indexes") at :2708
  • VERIFIED — SchemaVersion is 215 — StorageVersion.cs:20
  • VERIFIED — probe sentinel, newest-first arm returning 215, ladder entry with drop SQL — ViewerDataService.cs:1337, :1359, :1384, :1584-1588; RungRewindArtifacts.cs:508-515. 191 top-level probe expressions against 191 parameters, so the halves are in step
  • VERIFIED — MigrationProbeLadderTests covers the mapping only — :46-47 builds the arguments from Enumerable.Range and opens no connection
  • VERIFIED — the live AgPrimaryReplicaRungTests path runs the real probe SQL and proves a V215 store probes 215 — :194 executes StoreSchemaProbeSql, :54 and :72 assert StorageVersion.SchemaVersion
  • VERIFIED — no literal 214 pins existed — the only 214s are V214's own ordinals (RungRewindArtifacts.cs:503, ViewerKnownPatternEditorTests.cs:100), which CONTRIBUTING.md:270-273 says never move
  • VERIFIED — V184's InvestigationTables.CreateSql is not edited — absent from the changed files; :115-119 and :170-172 unchanged
  • VERIFIED — provisioning needs no change — provision-roles.sql:2824-2866 grants per table and per column; a PostgreSQL index carries no ACL
  • VERIFIED — predicate checks 1, 2, 3, 5, 6 — ProcedureStatsModuleDates.cs:44-46,48; InvestigationRequestStore.cs:185,225-229,523; InvestigationPracticeStore.cs:171,193; the ON DELETE SET NULL key at PgMigrations.cs:1591-1592
  • VERIFIED — predicate check 4, ReportsRequestsSql and ListSql filter and order on opened_at — InvestigationRequestStore.cs:485-486, :419, :445
  • VERIFIED — the plan-test pattern exists — Darling.Tests/PgStatementTextDueIndexLiveTests.cs
  • VERIFIED — V215 declared SetsTheFloor: false, multiple stays 5 — MigrationDataMovingRungCensusPins.cs:332-341; the pin at :396-426 counts floor rungs only, and :352 dedupes per rung, so six statements need one line
  • VERIFIED — no other test drops a V214 sentinel and then probes — PgInvestigationBriefSourceReaderLiveTests.cs:300-304 asserts on the reader, never on a probed version
  • VERIFIED — Lite declares no index on any investigation table — Lite/Database/Schema.cs:416-417
  • VERIFIED — index 6 leads on family_of so an equality probe can use it — the SET NULL action at PgMigrations.cs:1591-1592 updates on an equality, which implies NOT NULL and admits the partial index
  • UNVERIFIED — every wall-clock number in the table (127 to 5.5 ms, 310 to 0.5, and the four others) — a live measurement on a restored store copy with a synthetic year; a read-only tree cannot replay it
  • UNVERIFIED — the twice-applied scratch Timescale container, and "with the index an Index Scan, without it a Seq Scan" — needs live PostgreSQL; the second is what the suggested Seq Scan assertion would capture in CI
  • UNVERIFIED — Darling.Tests builds with 0 errors — no build was run here
  • UNVERIFIED — #6619 and #6635 also claim V215 — those branches are outside this checkout
  • UNVERIFIED — the tables hold hundreds of rows on production stores today — a production fact, not a tree fact
  • REFUTED — "No lock_timeout on V215. It matches V153." — V153 sets one, at PgMigrations.cs:206, as do V152 (:100), V154 (:219), V173 (:932) and V175 (:1008), and :83-97 records the failure it prevents. The precedent for omitting it is V104, V113 and V207, V207 being a partial index on this very table (ChangeWatchStore.cs:177). The decision is right; the rung cited for it says the opposite

Riskiest lines

  1. ViewerDataService.cs:1337 — if the sentinel, GetBoolean(190) or the trailing parameter were out of step, a fully-migrated store would probe 214 forever and the connect gate would refuse a current store permanently, no later upgrade changing the answer (CONTRIBUTING.md:277-279) — covered by AgPrimaryReplicaRungTests :54 and :72, live-only
  2. PgMigrations.cs:2307 — if the condition index's predicate were not implied by WatchedSql's WHERE the index would be dead weight on the 60k-calls-a-day read — covered by AgentTableIndexesV215LiveTests.TheFollowUpModuleRead_UsesTheConditionIndex_AndNotOnceItIsDropped, live-only
  3. RungRewindArtifacts.cs:515 — a wrong name on the sentinel's drop leaves it on disk and every store-below-rung-N rewind reads 215, the #5641 failure the ladder exists to prevent — covered by AgPrimaryReplicaRungTests:66 and ViewerKnownPatternEditorLivePostgresTests.OnAStoreBelowV214, live-only

Traps near this diff

  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — V215 adds no reader, tool, alert, description or state; CONTRIBUTING.md:310 scopes the rule to a rung plus a reader, and Lite/Database/Schema.cs:416-417 records why Lite's investigation tables carry no secondary index at all. The body's reasoning for this is wrong in three places; see the top-level note
  • "Ruling (#5833): tests are load-bearing. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled" — does not apply — the new test EXPLAINs the real read against a seeded store (AgentTableIndexesV215LiveTests.cs:66-67), and the census line (MigrationDataMovingRungCensusPins.cs:332-341) is a declaration an existing required pin demands, not a new census
  • "A test that encodes the lie: ACappedResult_WithholdsTheUnsatisfiedSummary_ handed the builder two facets at limit = 2 and asserted truncated - the >= limit inference as its own proof (#3699). Ask what defect a test would fail ON." — applies — AgentTableIndexesV215LiveTests.cs:71 fails on no defect in the rung: no plan can name a dropped index. It proves ExplainAsync re-plans, not that the index is what the measurement rests on. One-line fix suggested inline; non-blocking
  • "A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval." — does not apply — the seed's now() calls (AgentTableIndexesV215LiveTests.cs:58-62) are all transaction_timestamp() inside one statement, so one anchor owns the whole year
  • "Our wrong test is a bug we introduced, never a flake: a test that dev's own push broke failed ten unrelated PRs in a week (#5942)." — does not apply — the two tests this rung broke are fixed in its own commits (MigrationDataMovingRungCensusPins.cs:332, ViewerKnownPatternEditorTests.cs:226), and .github/ci-quarantine.txt is untouched

Divergence (blind read vs PR body)

Two places, both in the body's reasoning rather than in the rung.

  1. The lock_timeout precedent is inverted. The body offers V153 as the rung that omits one; V153 is one of the five that sets one, and PgMigrations.cs:83-97 records what it prevents - a 264-second aggregate refresh outlasting a flat wait, so the server's retryable 55P03 fires before the client-side command timeout cancels the statement. V207, a partial index on config.investigation_requests itself with no lock_timeout, makes the same point correctly and is one word away.

  2. The Lite rationale does not match Lite/. "Lite has the same tables and reads" is not so - no practice_of and no follow_up_condition (Schema.cs:430-465, DuckDbInvestigationStore.cs:146), so indexes 1 and 5 have no Lite counterpart to decline - and the ART-serves-equality argument argues for indexes on Lite's follow_up_of and family_of rather than against them. The reason the tree already holds, that an indexed column an UPDATE writes breaks a second update of the same row in one DuckDB transaction, is correct and stronger. Detail in the top-level note.

Elsewhere the body understates rather than overclaims: it says MigrationProbeLadderTests covers the mapping only, which is exactly what I found - the probe-SQL-to-arity alignment rides entirely on live tests, which is why riskiest line 1 is covered only when DARLING_TEST_PG is set.

Reviewer checklist

  • Run the live suite with DARLING_TEST_PG set: AgentTableIndexesV215LiveTests, AgPrimaryReplicaRungTests, ViewerKnownPatternEditorLivePostgresTests.OnAStoreBelowV214, and one rewind chain (CaggGroupIndexDropLiveTests or QueryStoreIntervalPartitionRungTests) to prove no chain now reads 215
  • On a restored store copy, confirm the planner picks index 6 inside the fk_investigation_reports_family check: EXPLAIN ANALYZE a retention-shaped DELETE in a rolled-back transaction, a cached SPI plan the suite cannot reach
  • Time the six builds on a copy of the largest live store, since no lock_timeout guards the SHARE lock against a long agent transaction on config.investigation_requests
  • Settle the rung number against dev at merge time: #6619 and #6635 both claim V215, and a gap is silent and unrepairable (CONTRIBUTING.md:283-293)

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 23, 2026 23:19

Claude posting for Erik Darling. Both divergences were in the body and are corrected: the lock_timeout precedent now cites V207, and the Lite rationale now uses the reason Schema.cs:416-417 records. No code change was needed. The non-blocking Seq Scan assertion goes in a test-only follow-up PR.

@erikdarlingdata
erikdarlingdata merged commit 760f29f into dev Oct 23, 2026
27 of 30 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/agent-table-indexes-v215 branch October 23, 2026 23:19
@github-actions github-actions Bot mentioned this pull request Oct 24, 2026
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