Repository navigation
Sweep the PostgreSQL AppliesTo gates too, and ship the IL guard first (#2532) - #2536
Conversation
…2532) #2530 taught the miss vocabulary the engine-KIND axis but wired only the engine half of the dispatch gate into it, so the PostgreSQL collectors' own AppliesTo gates were invisible to it and none of the eight get_pg_* reads asked the question at all. TargetsWithEngineKind now sweeps every target shape an engine kind permits: IsAurora is FIXED by the kind (nothing turns a stock PostgreSQL server into an Aurora one) while PostgresMajorVersion, PostgresVersionNum and IsInRecovery are VARIED, because an upgrade and a writer connection move those and a permanence claim over them would be the #2511 over-claim one engine over. IsCollectedOnEngineKind asks CollectorCatalog.AppliesTo over that sweep instead of EngineMatches alone. So pg_wait_stats and pg_statement_stats -- aurora_stat_system_waits() and aurora_stat_statements(), surfaces core PostgreSQL has in no version -- are permanent gaps on stock PostgreSQL, while pg_stat_io's PG16 floor and the writer-only autovacuum read keep the unavailable vocabulary. The message names the real reason. A foreign DIALECT is stopped by the dispatch gate's engine half; a same-dialect gap is the collector's own AppliesTo, and it borrows the edition axis's wording verbatim past the engine name. One sentence for both would tell a PostgreSQL operator that a PostgreSQL collector is not written for PostgreSQL. All eight get_pg_* reads now ask on the miss path. Five of them returned a bespoke all-clear -- no_holder, no_pending_maintenance, no_slots, no_io_activity, not_sampled -- which is the healthy answer on a PostgreSQL target and a fabricated one on a SQL Server target. The guard #2532 made the prerequisite ships with it: EveryFactAPostgresGateReads_IsVariedBySweepOrFixedByKind decodes the IL of every PostgreSQL AppliesTo and fails if the sweep leaves a fact it reads at its CLR default. Proven red by fixing IsInRecovery at false -- pg_autovacuum_stats' gate then reads a ConstantEverywhere fact and the guard names it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| ?? McpHelpers.Status( | ||
| "unavailable", | ||
| "No PostgreSQL query statistics for this server and window. If this server is SQL " | ||
| + "Server, use get_top_queries_by_cpu instead. If it is Aurora PostgreSQL, check that " | ||
| + "pg_stat_statements is installed in the database the collector connects to — on some " | ||
| + "clusters it exists only in the application database, not in postgres."); |
There was a problem hiding this comment.
Minor precision nit, in the spirit of what this PR is fixing elsewhere: the fallback text here is reached only when NotCollectedStatusAsync returns null, which (given pg_statement_stats gates on IsAurora) happens only when engine_kind is NULL/unrecognized, or when it's known to be aurora-postgres. In the latter case, "If this server is SQL Server, use get_top_queries_by_cpu instead" is already ruled out by the capability check that just ran — the comment above claims "the capability answer settles the first two branches of that sentence", but the string itself wasn't trimmed to reflect that, so an agent can still read a stale disjunct as live.
Same pattern in DarlingMcpPgWaitTools.cs (the "if it is PostgreSQL but not Aurora..." branch is unreachable once pg_wait_stats's own gate has already ruled that out) and in DarlingMcpPgWraparoundTools.cs ("means the server is SQL Server" is unreachable whenever engine_kind is known, since pg_wraparound_stats has no gate at all and the dialect gap is caught earlier). Low severity since it's phrased as a conditional rather than an assertion, but worth tightening for consistency with the rest of this PR's message discipline.
Review summaryReviewed the C#-only change to the PostgreSQL side of the engine-capability dispatch gate ( Correctness. The core mechanism checks out:
Parity. No issue: Lite has no PostgreSQL MCP tools at all (Lite is SQL Server-only), and Minor nit (left inline on No SQL/T-SQL changes in this PR, no security-relevant input handling changes, and no performance concerns (the sweep only runs on the already-slow miss path, over a small catalog). |
Review finding: the capability answer now rules out the dialect branch and the stock-PostgreSQL branch whenever the store knows the engine, but the fallback sentences still offered both as live possibilities. On an Aurora target "if this server is SQL Server, use get_wait_stats instead" is a disjunct the check above has already eliminated, and an agent can read a stale disjunct as a live one. Each fallback now names the two states that actually reach it -- a PostgreSQL target that has not collected in this window, and a row whose engine_kind is NULL -- and points at list_servers for the second, which is the read that answers it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch, and it is the same discipline this PR is about, so it should not have shipped with the disjuncts left in. Fixed in 006540c. All three fallbacks now name only the states that can still reach them:
Each now points at |
ReviewWent through the full diff ( Verified against source, not just the PR prose:
Nothing blocking found. This is a pure C# / test change with no missing-index-DMV concerns and no collector-query |
#2536 landed the PostgreSQL half of CapturePathByCollector while this branch added the SQL Server half, so the conflict is two sets of ADDITIONS to one table rather than two versions of anything. Kept both. Taking a side would have dropped one engine's noun phrases silently, and the reads that lost theirs do not fail -- they fall back to "the data this read is served from", which is true, explains nothing, and looks deliberate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Half of #2532: the PostgreSQL
AppliesToaxis, with the guard #2532 named as its prerequisite. The other half — wiring the remaining SQL Server reads on both SKUs — is a separate PR againstdev, because the read wiring is the user-visible half and should not wait on this.What was wrong
#2530 taught the miss vocabulary the engine-KIND axis, but only the engine half of the dispatch gate: "a SQL Server collector is never sent at a PostgreSQL target, and vice versa". The PostgreSQL collectors' own
AppliesTogates were invisible to it, and none of the eightget_pg_*reads asked the capability question at all.Five of those eight do not even use the
unavailablevocabulary — they return a bespoke all-clear. So a SQL Server target askedget_pg_xmin_horizongot:and
get_pg_autovacuum_healthreportedno_pending_maintenance— "No table on this server has dead tuples". That is not a weak answer about a mechanism the engine does not have; it is a fabricated clean bill of health, and it is exactly what an agent asking by name over MCP gets, because an agent has no tabs.The sweep
CollectorEngineCapability.TargetsWithEngineKind(engineKind)is the engine-kind twin ofTargetsWithEngineEdition, and it splits the facts the same way:IsAuroraIsAzureSqlDbhas on the edition axisPostgresMajorVersionPostgresVersionNumaurora_stat_resource_usage()needs 16.9+/17.5+ and is absent on 17.4)IsInRecoveryIsCollectedOnEngineKindnow asksCollectorCatalog.AppliesToover that sweep instead ofEngineMatchesalone. The SQL Server arm of the sweep varies the two Azure flags where the edition sweep fixes them, which is what keeps the axes from answering each other's question: a collector gated off on Azure SQL Database still runs on some SQL Server, so it is not a kind gap, and the edition axis is left to say so with the edition named.Measured over the shipped catalog (42 SQL Server / 8 PostgreSQL definitions):
pg_wait_statsandpg_statement_stats—aurora_stat_system_waits()andaurora_stat_statements(), surfaces core PostgreSQL has in no version — are permanent gaps on stock PostgreSQL, and on nothing else;pg_stat_io's PG16 floor and the writer-only autovacuum read are not, and keep theunavailablevocabulary that correctly sends an operator to look;The cross product deliberately includes incoherent pairs (major 16 alongside
server_version_num170005). A superset of the real target shapes can only find MORE shapes that pass a gate, so its error direction is under-claiming — the read keepsunavailable, which is the status quo. The opposite error is the one this whole mechanism exists to prevent, and a superset cannot make it.The message names the real reason
There are two shapes of kind gap and they are not the same thing:
The second borrows the edition axis's wording verbatim past the engine name, because they are the same finding. One sentence for both would tell a PostgreSQL operator that a PostgreSQL collector is not written for PostgreSQL, which is the sort of wrong that costs a reader their trust in the rest of the message.
The guard, which is why this could ship
#2532 said explicitly this must not ship without the PostgreSQL twin of #2518's IL-scanning sweep-dimension guard, and that machinery generalised exactly as it predicted —
FactsReadBySqlServerGatesbecameFactsReadByGatesOf(engine),RoleOfbecame parameterised over the shape families, and the two axes now share one decoder.EveryFactAPostgresGateReads_IsVariedBySweepOrFixedByKinddecodes the IL of every PostgreSQL definition'sAppliesTo, records whichCollectorTargetInfogetters it calls, and fails if any of them names a fact the kind sweep leaves at its CLR default. Both halves are read out of shipped artifacts — the facts from the gates' IL, the roles from running the sweep — so neither can be brought into line by editing the test.Alongside it:
ThePostgresGateScan_ReadsTheGatesTheSourceActuallyCarries(includingpg_blocking, which reads nothing, so the decoder is shown to discriminate — and an assertion that no SQL Server fact leaks in, because a filter that stopped filtering would look exactly like a guard that was working),TheKindSweep_IsTheFullCrossProductOfTheDimensionsItVaries,EveryTargetFact_LandsInExactlyOneDerivedKindRole, andAnUnrecognisedKind_SweepsNothing_AndStillClaimsNothing— the last because an empty sweep read as a gap set is the one failure here that would be catastrophic rather than merely wrong.Proven red
The suites are
net10.0-windowsand cannot run on macOS, so CI is the arbiter — but every assertion above was run locally against the real builtPerformanceMonitor.Collectorsin a throwawaynet10.0harness, over the shipped catalog and the shipped MCP source. 89 checks green.Then, red on demand:
IsInRecoveryatfalsein the sweep turnsEveryFactAPostgresGateReads_IsVariedBySweepOrFixedByKindred namingIsInRecovery — read by pg_autovacuum_stats, andIsInRecoveryslides fromVariedBySweeptoConstantEverywhere. Restoring it turns both green. That is the exact failure The engine-capability derivation is never proven to FOLLOW a gate that moves #2518 exists for, reproduced one engine over: without it,pg_autovacuum_statswould have been announced as a permanent gap on both PostgreSQL tokens.IsAurora,!IsAurora,PostgresMajorVersion >= 16,PostgresVersionNum >= 170005,!IsInRecovery,_ => falseand_ => true, and the answer follows every move in both directions. Only the two Aurora-shaped gates produce a permanent gap, and each produces it on exactly one token.Reads wired here
All eight
get_pg_*reads, Darling only — Lite has no PostgreSQL tools and no PostgreSQL target seam, andMcpEngineCapabilitystill passesengineKind: nullexplicitly, which I re-checked rather than assumed. Every call sits on the miss path, after the read came back empty, so a server whose registry row disagrees with its collected rows still gets its data.get_pg_blockingasks only whencaptures_total == 0: a window with captures is a window the collector ran in, so the engine cannot be in question there.CapturePathByCollectorgains the eight PostgreSQL noun phrases, so the messages name a surface rather than "the data this read is served from".Two tests changed meaning, deliberately
AShutAppliesToGate_IsNotAnEngineKindGap→AShutAppliesToGate_IsAnEngineKindGap_ButAFixableOneIsNot. The narrowing it protected — never report a fixable gate as permanent — is unchanged and is now enforced by the sweep on its own merits rather than by the axis refusing to look; the second half of the test is exactly that.EveryWiredRead_NamesARealCollectorThatIsGatedOffOnAzureSqlDb→…NamesARealCollectorWhoseCapabilityBranchCanFire. This is weaker and the doc comment says so. The Azure-specific claim was right while the edition axis was the only one; the kind axis makes every SQL Server collector a permanent gap on a PostgreSQL target, so demanding an Azure gap would refuse the wiring this issue is about. What survives is the property that matters at a call site — the branch is reachable — and the sharper claims stay in the derivation tests, over the gates rather than over the reads.EveryCapturePathEntry_NamesARealCollectorThatIsActuallyGatedSomewherekeeps its teeth by excluding the dialect half: an entry must name a collector its own gate shuts out somewhere, or one a shipped read asks about. A check phrased as "is this a gap somewhere" would now pass for any name anyone typed.One thing in #2532 that I could not confirm
#2532's own gate on this work was: "the claim to check before starting is whether any monitored PostgreSQL target is not Aurora — if none is and none is planned, this is theory." I could not re-measure the fleet from this session. On the stated fleet the stock-PostgreSQL claim buys nothing today; what it buys immediately is the other seven-eighths of this PR — the eight
get_pg_*reads no longer answering a SQL Server target with a fabricated all-clear — and the guard, which is what makes the sweep safe to have shipped at all.Closes nothing on its own; #2530 stays open for the UI waves, and #2532 stays open for the read wiring.
🤖 Generated with Claude Code