Repository navigation
Fixes #3070 - #3071
Fixes #3070#3071
Conversation
Review summaryThis PR wires up an already-existing, already-shared reader ( Verified as correct:
One minor nit posted inline: the new "Plan Capture Readiness" panel is the only windowed No other correctness, security, or Lite/Darling parity issues found. |
|
Reviewed. This is a code-only change (no Correctness
Lite/Darling parity
Security
Performance
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. |
get_pg_plan_capture_readinessnow exists, and the PostgreSQL read-coverage ratchet is at zero.The read
A tool wrapper, not new SQL.
DarlingPgPlanCaptureReadinessReader.GetPgPlanCaptureReadinessAsyncalready held the latest-per-facet reduction and the causal ordering, already shared with the WPF tab per #2530, soGetPgPlanCaptureReadinessinDarlingMcpPgPlanToolscalls it and projects it. Same parameter conventions, server resolution and window validation asGetPgPlansbeside 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 wasViewerServerTab.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_messagesis overridable per role and per database, so a globalde_DEwith the monitoring role atCreads 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, soTheDetailTravelsVerbatim_BecauseItsCaveatsAreAtTheEndOfItasserts character-for-character equality.Three shape decisions worth naming:
UnsatisfiedFacetsAsyncin the same file filters tois_satisfied IS NOT TRUEand selectsfacet, observedonly, 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.readyboolean. An unmetplan_attributionstill captures plans and merely orphans them;message_localeis 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.limitremains 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/readdispatch entry, its catalog descriptor (withPAsOf(), so the anchor is reachable from the web surface), aCapturePathByCollectornoun phrase, and aPlan Capture Readinesspanel on the PostgreSQL Activity tab. Activity rather than the Viewer's Vacuum tab because that is whereget_pg_plans,get_pg_deadlocksandget_pg_deadlock_detailalready 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.DarlingMcpPgPlanToolswas already registered with the host, so no newWithGeminiCompatibleToolsline, and the instructions census moves 137 → 138 / 51 → 52 / thirty-two → thirty-three PostgreSQL reads.CapturePathByCollectornames the settings, not "plan capture readiness". The gap this read can have is not that plans are missing —pg_plan_capturecovers that one — it is that nothing has told the reader whether the target could capture a plan, so the phrase describes theauto_explainpreload, threshold, format and log-prefix settings plus thelc_messageslocale. Without an entry thenot_collectedmessage falls back to "the data this read is served from", which is true and tells an operator nothing.The ratchet
KnownUnreadabledrops 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 ondev, 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 aget_pg_plan_capture_readinesstool that did not exist. One of those six,message_locale(#3061), is a precondition forget_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.
CollectorForReadgains the read-to-collector entry it needs — that map is asserted to cover every servedget_pg_*read, so an unmapped one is silently skipped by its consumers rather than caught by them.Lite
A
KnownLiteMissingMcpToolsentry, 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.StoredCollectorsfilterspg_plan_capture_readinessout of every generation loop, and Lite passesengineKind: nullexplicitly. The entry also says there is no near-twin to confuse it with — the facets areauto_explainpreconditions 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
detailinto the existing empty-result answers meansUnsatisfiedFacetsAsyncis 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 toget_pg_deadlocksandget_pg_deadlock_detailto stay consistent, and more wherever the locale facet reaches next. It makes already-long status strings longer. And it still leaves thedetailcolumn 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
SchemaVersionbump.collect.pg_plan_capture_readinessis V87 and the reader already reads it. Code-only, the same as #3068.Verification
Darling.TestsandLite.Testsarenet10.0-windowsand cannot run on macOS, so the real test files were compiled into a throwawaynet10.0xunit v3 host staged inside a gitignoredbin/(assembly namedDarling.Testsfor theInternalsVisibleToedge, output inside the tree soParitySource.RepoRoot()andReadRepoFile'sCallerFilePathwalk resolve to this checkout). 217 tests, 0 failed, 6 skipped — all six skips are live-Postgres classes wantingDARLING_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 testswent red onEveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFirefor the missingCapturePathByCollectorentry (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.PgRegistryPanelPlacementTestsis not in that host and could not be: it references the WPF Viewer assembly, so adding it forcesnet10.0-windowsand nothing runs on macOS. It is unaffected by construction — it comparesViewerPostgresTabs.Allagainst 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:
ThePostgresCollectorsWithNoServedRead_OnlyEverShrink, aloneBuildReadinessJson, which is a stronger result than a red and is recorded as that rather than counted as a test failureKnownLiteMissingMcpToolsentry removedLiteMissingMcpTools_MatchTheRatchetAllowListDarlingInstructionsCensus_MatchesTheScannedInventoryEveryPostgresRead_IsReachableFromThePostgresRegistry_AndFromNoOtherTabCollectorForReadentry removedEveryAuroraOnlyPostgresRead_SitsOnATabThatSaysSoPAsOf()dropped from the catalog entryEveryDispatchedReadWhoseToolTakesAnAnchor_AdvertisesItInTheCatalogDateTime.UtcNowinstead of the resolved anchorEveryToolThatTakesTheAnchor_ActuallyUsesItdetaildropped from the projectionEveryReadinessFacet_CarriesItsRemedy_InTheReadersCausalOrderEveryReadinessFacet_CarriesItsRemedy_InTheReadersCausalOrderACappedResult_WithholdsTheUnsatisfiedSummary_RatherThanDescribingThePagedetailtruncated to 200 charactersTheDetailTravelsVerbatim_BecauseItsCaveatsAreAtTheEndOfItCapturePathByCollectorentry removedEveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFireEvery mutation was restored and the restore verified byte-identical rather than by diffstat.
Not verified
not_collected/precondition/emptybranches were read for correctness against their siblings but not exercised.formatvalues are in the shippedFORMAT_OPTIONSvocabulary, but no browser saw it.Darling PostgreSQL testsand the Windowsbuildjob 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. Thedetailcolumn 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 aget_pg_plan_capture_readinesstool 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 unmetplan_attributionstill captures plans and merely orphans them. Every PostgreSQL collector now has a served read.