Skip to content

Fixes #3070 - #3071

Merged
erikdarlingdata merged 6 commits into
devfrom
fix/pg-plan-capture-readiness-read
Sep 6, 2026
Merged

erikdarlingdata merged 6 commits into
devfrom
fix/pg-plan-capture-readiness-read

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 6, 2026 •

Copy link
Copy Markdown
Owner

get_pg_plan_capture_readiness now exists, and the PostgreSQL read-coverage ratchet is at zero.

The read

A tool wrapper, not new SQL. DarlingPgPlanCaptureReadinessReader.GetPgPlanCaptureReadinessAsync already held the latest-per-facet reduction and the causal ordering, already shared with the WPF tab per #2530, so GetPgPlanCaptureReadiness in DarlingMcpPgPlanTools calls it and projects it. Same parameter conventions, server resolution and window validation as GetPgPlans beside it.

It carries detail, verbatim, which is the point. That column is the per-facet consequence and remedy, written against what the collector actually saw, and its only reader was ViewerServerTab.Postgres.cs — so on a Linux host and to every agent the remedy did not exist.

Verbatim matters because the load-bearing clauses are at the far end of those strings. message_locale's satisfied arm spends its last sentence stating its own limit — lc_messages is overridable per role and per database, so a global de_DE with the monitoring role at C reads satisfied about a server whose backends translate — and its unset arm exists to say that empty is UNKNOWN rather than safe, because PostgreSQL then takes its language from the server process environment and no query can see that. A projection that shortened these for readability would drop exactly the caveats that stop a reader over-trusting a satisfied row, so TheDetailTravelsVerbatim_BecauseItsCaveatsAreAtTheEndOfIt asserts character-for-character equality.

Three shape decisions worth naming:

  • Every facet is emitted, satisfied ones included. UnsatisfiedFacetsAsync in the same file filters to is_satisfied IS NOT TRUE and selects facet, observed only, so it structurally cannot answer "what is the state of this server's plan capture" — which is the question somebody has when the plans are missing.
  • No single ready boolean. An unmet plan_attribution still captures plans and merely orphans them; message_locale is about every target-side log read rather than about capture. One flag would have to pick a meaning and be wrong under the other, which is the collapse the collector splits its rows to avoid. The unsatisfied facets are named instead.
  • The unsatisfied summary is withheld when the result is capped, with a sentence saying why — Nine PostgreSQL collectors are readable only from the Windows Viewer — the MCP and web surfaces cannot see them #2629's lesson, on a read whose small row count makes the guard look unnecessary while limit remains the caller's.

The two remedy strings #3068 rewrote to name the collector now name the read as well — they are what the issue was filed on, and they asked for exactly this tool before it existed.

Registration: the /api/read dispatch entry, its catalog descriptor (with PAsOf(), so the anchor is reachable from the web surface), a CapturePathByCollector noun phrase, and a Plan Capture Readiness panel on the PostgreSQL Activity tab. Activity rather than the Viewer's Vacuum tab because that is where get_pg_plans, get_pg_deadlocks and get_pg_deadlock_detail already live — the reads whose empty results this one explains. The deadlock panel's empty text pointed at "the plan-capture readiness panel" and there was no such web panel; it now names the one above it. DarlingMcpPgPlanTools was already registered with the host, so no new WithGeminiCompatibleTools line, and the instructions census moves 137 → 138 / 51 → 52 / thirty-two → thirty-three PostgreSQL reads.

CapturePathByCollector names the settings, not "plan capture readiness". The gap this read can have is not that plans are missing — pg_plan_capture covers that one — it is that nothing has told the reader whether the target could capture a plan, so the phrase describes the auto_explain preload, threshold, format and log-prefix settings plus the lc_messages locale. Without an entry the not_collected message falls back to "the data this read is served from", which is true and tells an operator nothing.

The ratchet

KnownUnreadable drops to 0 and the comment states the current reason rather than the eroded one. It had read "its output is a single row of configuration state, a panel rather than a question anyone asks an agent", and no clause of that survived: six facet rows on dev, each with a different remedy in its own column, that column with no non-Windows reader, and two remedy strings elsewhere in the service already directing callers to a get_pg_plan_capture_readiness tool that did not exist. One of those six, message_locale (#3061), is a precondition for get_pg_deadlocks, whose empty result is the healthy state — so the exemption was hiding the one fact that separates a quiet server from a read that cannot see anything. The method's doc comment keeps the exemption mechanism — an exemption is still possible, deliberately, with the collector named — and records that the one exemption taken did not survive examination.

The second assertion is kept rather than deleted even though it now says nothing the first does not: the pair is what makes this a ratchet rather than a cap, and dropping the lower-it half at the bottom is how the ground stops being held.

CollectorForRead gains the read-to-collector entry it needs — that map is asserted to cover every served get_pg_* read, so an unmapped one is silently skipped by its consumers rather than caught by them.

Lite

A KnownLiteMissingMcpTools entry, alongside the other Darling-only PostgreSQL reads and for the same architectural reason rather than as a porting to-do: Lite has no PostgreSQL target and cannot acquire one, DuckDbSchemaGenerator.StoredCollectors filters pg_plan_capture_readiness out of every generation loop, and Lite passes engineKind: null explicitly. The entry also says there is no near-twin to confuse it with — the facets are auto_explain preconditions and a log-message locale, and Query Store, the nearest thing conceptually, is a database-scoped feature with its own health read rather than a set of preload-only server GUCs.

Why not inline the detail instead

Rejected on merits rather than left unconsidered, so it is not re-proposed. Putting detail into the existing empty-result answers means UnsatisfiedFacetsAsync is still the only path to these rows, and it shows only the unsatisfied facets — a reader can never see the satisfied ones or the readiness picture as a whole. It needs parallel edits to get_pg_deadlocks and get_pg_deadlock_detail to stay consistent, and more wherever the locale facet reaches next. It makes already-long status strings longer. And it still leaves the detail column with no direct reader, when the house rule is that the remedy travels with the observation.

No rung

No migration rung, no schema change, no SchemaVersion bump. collect.pg_plan_capture_readiness is V87 and the reader already reads it. Code-only, the same as #3068.

Verification

Darling.Tests and Lite.Tests are net10.0-windows and cannot run on macOS, so the real test files were compiled into a throwaway net10.0 xunit v3 host staged inside a gitignored bin/ (assembly named Darling.Tests for the InternalsVisibleTo edge, output inside the tree so ParitySource.RepoRoot() and ReadRepoFile's CallerFilePath walk resolve to this checkout). 217 tests, 0 failed, 6 skipped — all six skips are live-Postgres classes wanting DARLING_TEST_PG; every test named below ran, confirmed by enumerating the discovered list.

The first CI run caught something the host did not, and the honest account is that the hole was in which pins I had chosen rather than in the technique: Darling PostgreSQL tests went red on EveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFire for the missing CapturePathByCollector entry (7786 tests, 1 failure). Rather than fix just that test, every pin in either suite that scans the Darling MCP directory or the wired-read set is now in the host — EngineCapabilityMissTests, RuntimePreconditionMissTests, DarlingWebEndpointsTests, McpMissMessageParityPinTests — which is what took it from 58 tests to 217.

PgRegistryPanelPlacementTests is not in that host and could not be: it references the WPF Viewer assembly, so adding it forces net10.0-windows and nothing runs on macOS. It is unaffected by construction — it compares ViewerPostgresTabs.All against the XAML and the load path, and this change touches none of the three. CI runs it.

Ten mutations, each against a committed copy, each compiled, each red on a different named assertion:

mutation red
the read unserved (dispatch, catalog and panel removed, tool method kept) ThePostgresCollectorsWithNoServedRead_OnlyEverShrink, alone
the whole read removed including the tool method does not compile — the wire pins reference BuildReadinessJson, which is a stronger result than a red and is recorded as that rather than counted as a test failure
the KnownLiteMissingMcpTools entry removed LiteMissingMcpTools_MatchTheRatchetAllowList
the census reverted to 137 / 51 / thirty-two DarlingInstructionsCensus_MatchesTheScannedInventory
the web panel removed EveryPostgresRead_IsReachableFromThePostgresRegistry_AndFromNoOtherTab
the CollectorForRead entry removed EveryAuroraOnlyPostgresRead_SitsOnATabThatSaysSo
PAsOf() dropped from the catalog entry EveryDispatchedReadWhoseToolTakesAnAnchor_AdvertisesItInTheCatalog
the tool windowing from DateTime.UtcNow instead of the resolved anchor EveryToolThatTakesTheAnchor_ActuallyUsesIt
detail dropped from the projection EveryReadinessFacet_CarriesItsRemedy_InTheReadersCausalOrder
the facets re-sorted unsatisfied-first EveryReadinessFacet_CarriesItsRemedy_InTheReadersCausalOrder
the truncation guard removed from the summary ACappedResult_WithholdsTheUnsatisfiedSummary_RatherThanDescribingThePage
detail truncated to 200 characters TheDetailTravelsVerbatim_BecauseItsCaveatsAreAtTheEndOfIt
the CapturePathByCollector entry removed EveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFire

Every mutation was restored and the restore verified byte-identical rather than by diffstat.

Not verified

  • No live PostgreSQL target was reachable. Nothing here executed against a monitored server or a seeded store: the reader's SQL is unchanged and already proven, and what is new is the projection over its rows, which is what the wire pins cover. The not_collected / precondition / empty branches were read for correctness against their siblings but not exercised.
  • The web panel was not rendered. It is pinned by the same text scans as the rest of the registry and its format values are in the shipped FORMAT_OPTIONS vocabulary, but no browser saw it.
  • Darling PostgreSQL tests and the Windows build job are CI's to run.

CHANGELOG entry text

Not applied here — [Unreleased], under Added:

  • get_pg_plan_capture_readiness, and the PostgreSQL read-coverage ratchet reaches zero ([pg_plan_capture_readiness is the read-coverage ratchet's only exemption, and every clause of its justification has stopped being true #3070]) - whether a PostgreSQL target can capture execution plans at all, facet by facet, with the remedy for each step that is not in place. The detail column is the per-facet consequence and fix and its only reader was the WPF tab, so on a Linux host and to every agent the remedy did not exist. It was the ratchet's single exemption, held to be "a single row of configuration state"; it is one row per facet, each with a different remedy, and two remedy strings elsewhere in the service already pointed callers at a get_pg_plan_capture_readiness tool that was never written. Every facet is reported whether or not it is satisfied - a list of only the failures cannot show that capture is working - in causal rather than alphabetical order, and there is deliberately no single ready/not-ready verdict, because an unmet plan_attribution still captures plans and merely orphans them. Every PostgreSQL collector now has a served read.

Comment thread Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/server-tabs.js Outdated
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review summary

This PR wires up an already-existing, already-shared reader (DarlingPgPlanCaptureReadinessReader, used by the WPF viewer) into a new MCP tool (get_pg_plan_capture_readiness), a web-dashboard panel, and updated remedy text — no new SQL and no migration rung, which matches the "code-only" claim in the description.

Verified as correct:

  • GetPgPlanCaptureReadiness follows the same resolve/validate/try-catch shape as the sibling GetPgPlans tool (server resolution, window validation, limit validation, engine-gate fallback on exception).
  • truncated = rows.Count >= limit in BuildReadinessJson matches the same convention already used in DarlingMcpPgDeadlockTools, DarlingMcpPgServerStateTools, and DarlingMcpPgIndexTools — not a new pattern, and with only 6 possible facet rows against a default limit of 25 it's a non-issue in practice.
  • Web panel placement (Activity tab, between Captured Plans and Deadlocks) matches the PR description's stated rationale and the surrounding empty-text updates are consistent (the deadlock panel's empty text now correctly points at "the Plan Capture Readiness panel above").
  • The tool-count census in DarlingMcpInstructions.cs (137→138 tools, 51→52 Darling-only) is enforced by a dynamic scan-based test (CrossAppMcpToolInventoryPinTests.DarlingInstructionsCensus_MatchesTheScannedInventory) rather than hardcoded, so it can't silently drift; the prose "thirty-two"→"thirty-three" PostgreSQL-reads count also checks out against the actual get_pg_* dispatch entries (33).
  • The ThePostgresCollectorsWithNoServedRead_OnlyEverShrink ratchet drop to KnownUnreadable = 0 is computed dynamically from CollectorCatalog.All vs. the real dispatch table, so it can't be gamed by a stale literal.
  • Lite parity: KnownLiteMissingMcpTools gets the new entry with a documented architectural reason (no PostgreSQL target in Lite), consistent with how every other Darling-only PostgreSQL tool is already handled — this isn't a parity gap, since Lite structurally cannot serve this read (DuckDbSchemaGenerator.StoredCollectors filters PostgreSQL-only collectors out generically, unchanged by this PR).
  • No SQL injection / XSS surface: all params are bound via Npgsql parameters, and the new web panel's detail/observed columns (server-controlled strings, e.g. log_line_prefix) go through the file's existing generic column renderer, which the file's own header confirms uses the text-node path (never innerHTML).

One minor nit posted inline: the new "Plan Capture Readiness" panel is the only windowed table() call in server-tabs.js whose subtitle doesn't include ctx.label, breaking this file's otherwise-universal convention of showing the active time range on every panel that takes one.

No other correctness, security, or Lite/Darling parity issues found.

@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Reviewed. This is a code-only change (no .sql files touched, so the T-SQL style conventions don't apply here) that wraps an already-existing, already-shared reader (DarlingPgPlanCaptureReadinessReader, in place since #2530) in a new MCP tool + web panel. Went through it for correctness, Lite/Darling parity, and security.

Correctness

  • GetPgPlanCaptureReadiness follows the exact same shape as the neighboring GetPgPlans (server resolution, window/limit validation, try/catch → NotCollectedStatusAsync → generic error), and NoReadinessStatusAsync's three-way fallback (engine gate → runtime precondition → "empty is not healthy here" message) is consistent with the rest of the file.
  • BuildReadinessJson's truncated = rows.Count >= limit uses the same (slightly conservative, off-by-one-safe-side) convention as every other truncation check in this codebase (DarlingMcpPgIndexTools, DarlingMcpPgServerStateTools, DarlingMcpPgDeadlockTools, etc.), so it's not a new inconsistency.
  • Withholding unsatisfied_facets on a capped result (rather than computing it over a partial page) matches the Nine PostgreSQL collectors are readable only from the Windows Viewer — the MCP and web surfaces cannot see them #2629 pattern already used elsewhere.
  • DarlingRuntimePrecondition.StatusAsync is a generic collector-name lookup against collection_log, so passing "pg_plan_capture_readiness" needs no new registry entry — confirmed correct.

Lite/Darling parity

  • Correctly not ported to Lite, and the ratchet test (KnownLiteMissingMcpTools) was updated accordingly. Verified the stated reasons hold: DuckDbSchemaGenerator.StoredCollectors filters to TargetEngine == SqlServer only, so Lite never materializes any PostgreSQL collector tables, pg_plan_capture_readiness included. No parity drift.
  • CollectorForRead and ThePostgresCollectorsWithNoServedRead_OnlyEverShrink (web tab registry pins) and the census counts in DarlingMcpInstructions.cs were all updated together and consistently (137→138, 51→52, thirty-two→thirty-three).

Security

  • No new user input paths beyond the existing server_name/hours_back/limit/as_of pattern, all validated through the shared McpHelpers validators before use; the readiness SQL itself is unchanged (pre-existing, parameterized). No secrets, file, or process handling introduced.

Performance

  • No new queries — this reuses the existing shared reader's DISTINCT ON query, already used by the WPF tab. No regression risk.

Didn't find anything to flag. The self-review in the PR description (mutation table, verbatim-restore verification) matches what's actually in the diff.

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