#2138 Phase 0: CPU-primary plan-regression scoring + absolute-spend noise floor

[erikdarlingdata]

Phase 0 of #2138 (gaps 1 + 2 from the gap analysis on the issue): make the PLAN_REGRESSION verdict trustworthy enough to someday act on, while everything stays advise-only.

What changed

CPU is now the primary signal. The old score was GREATEST(cpu ratio, duration ratio), so a plan whose CPU never moved could fire on duration alone — but duration is confounded by blocking, IO waits, and machine contention that no plan choice caused, exactly the false positive a plan-forcing bot must never act on. Now:

• A CPU regression scores at its own ratio (>= 2x fires, unchanged).
• A duration-dominant regression fires only when EXTREME (>= 4x) and corroborated by at least mild CPU worsening (>= 1.25x), scored at half the duration ratio so it competes honestly with CPU-detected rows.

A 10-CPU-second absolute-spend floor. The latest plan must have burned >= 10 CPU-seconds across the 14-day comparison window. This is a noise gate, not an importance ranking — a 12x ratio on a query costing 12ms/run over 100 runs is sampling jitter, not a finding. Magnitude ranking stays with the factor and the scorer.

Fact and drill-down agree by construction. The regressed-queries drill-down kept its OWN copy of the old GREATEST in both SKUs — left alone, it could surface rows the fact never counted. Both drill-downs now run the identical CASE + floor.

Where

All four SQL sites: PgFactCollector.QueryPerf.cs + PgDrillDownCollector.Queries.cs (Darling), DuckDbFactCollector.QueryPerf.cs + DrillDownCollector.Queries.cs (Lite). The regressed_dimension metadata stays honest for the new path: a duration-fired row reports dimension 2.

Pins

Split-signal seeds (CPU and duration moved independently) in the DuckDB suite and mirrored as new arms of the live-Postgres twin, so the REAL Postgres SQL is exercised too:

• duration-only (CPU flat, duration 5x) does NOT fire — and the drill-down returns empty for the same seed;
• extreme duration (6x) with mild CPU corroboration (1.5x) fires at 3.0 (dur/2), not 6.0;
• the same 12x CPU ratio that passes everywhere else does NOT fire when the query burns only 1.2 CPU-seconds across the whole window;
• all classic 12x/3x replica-split assertions unchanged and still passing gates (existing seeds regress on both signals and spend 30–120 CPU-seconds).

Existing consumers are unaffected mechanically: scorer threshold formula, remediation generation, and force-plan replica-scope logic all consume the factor/rows downstream of this filter.

What this is NOT

No behavior toward forcing plans. Phases 1–2 (evict-first, live-hot targeting, self-review windows, auto-unforce ledger, write-credential tier) remain design-gated on the issue.

Part of #2138 — leave the issue open.

[claude]

regressed_dimension metadata can now be mislabeled for CPU-primary rows.

The new CASE gives the CPU branch precedence: whenever cpu_ratio >= 2, the row scores as cpu_ratio regardless of how large dur_ratio is (duration is only ever consulted when the CPU branch didn't already fire). But the downstream attribution in CollectPlanRegressionFactsAsync (same file, ~line 392-394) still does:

var cpuRatio = bestCpu > 0 ? latestCpu / bestCpu : 0.0;
var durRatio = bestDur > 0 ? latestDur / bestDur : 0.0;
worstDimension = cpuRatio >= durRatio ? 1 : 2; // 1 = cpu, 2 = duration

That logic was correct under the old GREATEST(cpu_ratio, dur_ratio) scoring (the larger raw ratio was the one that set the factor), but it's no longer correct now that the SQL picks CPU by precedence, not by magnitude. Concrete failure: cpu_ratio = 2.5, dur_ratio = 10 (e.g. a genuine CPU regression that also picked up extra blocking). The CASE fires the CPU branch — regression_factor = 2.5 — but worstDimension computes 2.5 >= 10 → false → reports dimension 2 (duration), mislabeling a CPU-caused regression as duration-caused. The PR description says "a duration-fired row reports dimension 2," but the converse isn't guaranteed: a CPU-fired row can now report dimension 2 too.

Fix: derive worstDimension from which CASE branch actually fired (cpuRatio >= 2 ? 1 : 2), not from comparing the two raw ratios. Same bug exists in Lite/Analysis/DuckDbFactCollector.QueryPerf.cs (~line 390-392) — parity is preserved, but both sides need the fix. None of the new split-signal tests pin regressed_dimension, which is how this slipped through.

[claude]

Same issue as the Darling counterpart (PgFactCollector.QueryPerf.cs): the CASE now gives CPU precedence over duration, but worstDimension in CollectPlanRegressionFactsAsync below (~line 390-392) still infers the dimension by comparing raw cpuRatio >= durRatio, which matched the old GREATEST scoring but not this one. A row with cpu_ratio = 2.5, dur_ratio = 10 fires the CPU branch (regression_factor = 2.5) yet gets regressed_dimension = 2 (duration) because 2.5 < 10. Should derive the dimension from which branch fired (cpuRatio >= 2 ? 1 : 2) instead.

[claude]

Reviewed the diff (all 4 SQL sites: Darling fact + drill-down, Lite fact + drill-down, plus the new tests).

Verified correct:

• The CASE precedence (CPU branch first, duration branch requires dur_ratio >= 4 AND cpu_ratio >= 1.25, scored at dur_ratio / 2) is applied identically across all four SQL sites — Lite/Darling parity is intact for the scoring logic itself.
• The 10-CPU-second floor (latest_total_cpu_us >= 10000000, units are microseconds) is the same literal in all four sites and the arithmetic checks out against the new test seeds (e.g. the below-floor test: 100 execs × 12,000us = 1,200,000us < 10,000,000us → correctly excluded).
• Div-by-zero is guarded consistently via NULLIF(..., 0), same as the prior GREATEST version — no new null-handling regressions.
• New pinned tests' expected values match the SQL arithmetic (duration-only 5x doesn't fire; 6x duration + 1.5x CPU fires at 3.0; classic 12x seeds still fire).

One correctness bug found (left as inline comments on both sites): regressed_dimension attribution in CollectPlanRegressionFactsAsync (Darling PgFactCollector.QueryPerf.cs and Lite DuckDbFactCollector.QueryPerf.cs) still infers CPU-vs-duration by comparing raw ratio magnitudes (cpuRatio >= durRatio ? 1 : 2). That was correct when the SQL scored via GREATEST (the bigger ratio was definitionally what set the factor), but it's stale now that the CASE picks CPU by precedence, not magnitude. A row where cpu_ratio = 2.5 and dur_ratio = 10 fires the CPU branch (regression_factor = 2.5, correctly CPU-caused) but gets mislabeled regressed_dimension = 2 (duration) since 2.5 < 10. This is a pre-existing bit of C# left un-updated by this SQL change, present identically on both SKUs (parity preserved, but both wrong), and none of the new split-signal tests exercise regressed_dimension so it wasn't caught. Given this feeds a future auto-force-plan bot's judgment of why a plan regressed, worth fixing before merge.

No SQL injection, secrets, or file/network/process concerns — all queries are parameterized. No missing-index suggestions per your note. Nothing else stood out as a Lite/Darling drift or performance regression.

[erikdarlingdata]

Good catch on regressed_dimension — confirmed real: the magnitude comparison was only correct when the score WAS the max of the two ratios. Fixed in 5dd9e75 by deriving the dimension from which CASE branch fired (cpuRatio >= 2 ? 1 : 2 — exhaustive, since a surviving row must have fired one branch), both SKUs, and dropped the now-dead latestDur/bestDur locals so nobody trusts them later. Pinned with the exact failure case you described (cpu 2.5x + dur 10x → factor 2.5, dimension cpu) plus a dimension assert on the duration-fired arm, in the DuckDB suite and the live-Postgres twin.

[claude]

Review summary

Reviewed the CPU-primary plan-regression scoring change against dev. This is a solid, well-scoped Phase 0 change — I traced the scoring math by hand against all four new test arms (duration-only-no-fire, extreme-duration-with-corroboration at dur/2, CPU-precedence dimension labeling, and the 10-CPU-second noise floor) and it checks out exactly as described.

Correctness

• The CASE scoring (CPU branch >= 2, duration branch >= 4 AND corroboration >= 1.25 scored at /2) is implemented identically in the fact query and the drill-down query, in both SKUs — so fact and drill-down can no longer disagree, which was the stated goal.
• The regressed_dimension derivation in C# (cpuRatio >= 2 ? 1 : 2) correctly mirrors the SQL branch precedence rather than comparing raw ratio magnitudes — verified this matches the CASE's short-circuit behavior for the "cpu 2.5x / duration 10x" mixed-signal case the second commit's review-catch fixes.
• The new latest_total_cpu_us >= 10000000 noise floor is applied consistently in both the fact and drill-down queries, computed as execs * cpu_per_exec (recovers the total from the weighted per-exec average correctly).
• Traced the one seed with an exact CPU tie (DurationOnlyRegression_DoesNotFire, good/bad CPU both 100,000µs) through the cheapness ROW_NUMBER() tie-break: whichever way the tie resolves, the row either gets excluded by l.query_plan_hash <> b.query_plan_hash or fails both CASE branches, so the assertion holds regardless — not a flakiness risk.

Lite/Darling parity — checked byte-for-byte: the CASE expression, the floor literal, the regressed_dimension derivation, and the test-seed plumbing (avgDurUs/goodDurUs/badDurUs threaded through SeedRowAsync/SeedAsync) are identical across both SKUs. Parameter-position mapping in both seed helpers' raw SQL was checked against the column lists and lines up correctly.

Security — no dynamic SQL, all values parameterized ($n / DuckDBParameter) as before. No new I/O, file, network, or process surface.

Performance — the added CASE/floor are cheap CTE-level predicates over already-bounded (LIMIT 20/LIMIT 5) result sets; no new scans or missing-index concerns.

Left one inline nit (low severity, optional cleanup): latest_dur/best_dur are still selected in the fact query in both SKUs but are no longer read now that dimension attribution derives from cpuRatio alone — harmless, just dead columns.

No blocking issues found.

[claude]

Minor/optional: now that regressed_dimension is derived from cpuRatio >= 2 alone (rather than comparing cpuRatio vs durRatio), the latest_dur/best_dur columns a few lines up (l.dur_per_exec AS latest_dur, b.dur_per_exec AS best_dur) are no longer read anywhere in this method — the latestDur/bestDur locals that used to consume them were removed. They're still selected all the way through the CTE and final SELECT list, just dead weight now (harmless, bounded to 20 rows, but could be dropped). Same applies to the Lite mirror in DuckDbFactCollector.QueryPerf.cs.