Repository navigation
Plan analysis: rule 38 reads the server's edition and MAXDOP at every entry point (#4530) - #4601
Merged
Merged
Conversation
Ports PerformanceStudio's AnalyzerConfig/RulesConfig (disabled rules, severity overrides) and the top-level ServerMetadata fields into PerformanceMonitor.PlanAnalysis, adds PlanWarning.RuleNumber, and adds Analyze/Run overloads that take (config, serverMetadata, cancellationToken) and thread both down to the statement and node walks. The existing overloads still forward to AnalyzerConfig.Default and a null metadata, and no rule reads either new parameter yet, so analyzer output is unchanged.
…ng framework as legacy (#4566)
…ing (#4535) Ports erikdarlingdata/PerformanceStudio's severity-override pass: overrides now find a warning's rule via PlanWarning.RuleNumber instead of matching WarningType against a rule-to-name table, and the engine's own warnings (Source == SqlServer) are always skipped. Also ports PlanStatements.EnumerateAllWithContainer, with EnumerateAll delegating to it so the plain and container-carrying walks stay one walk.
…5-analyzer-config
…5-analyzer-config
#4535, #4566) The plumbing step's RuleNumber_DefaultsToNull_UntilLaterStepsStampIt pinned the state partway through the per-rule configuration rollout (only rule 3 stamped). Every step that stamps a rule has landed on this branch, so that pin is wrong by design now. Replaces it with PlanSync4535RuleNumberCensusTests: a code census over every new PlanWarning construction in PlanAnalyzer.cs (each must set RuleNumber, except a SqlServer-sourced initializer), a runtime census over the existing plan fixtures (every analyzer finding carries a non-null RuleNumber), and a cross-step check that disabling every stamped rule number leaves zero analyzer findings.
…into plan-sync/4530-entry-points
Adds three tests the earlier #4530 steps skipped: - a live Postgres pin that seeds collect.server_properties and collect.server_config on a migrated store and asserts DarlingServerMetadataReader.ReadAsync returns the edition and MAXDOP, then runs the same plan XML through the analysis formatter and confirms rule 38 (Standard Edition DOP 2 limitation) gives its Warning, plus a no-rows case that stays on the Info branch; - a drill-down pin: a PLAN_WARNING finding's drill-down surfaces the rule-38 Warning when PgDrillDownCollector runs against a seeded Standard Edition / MAXDOP 8 server; - a Lite.Tests pin (build-only on macOS) for LocalDataService.GetServerMetadataForPlanAnalysisAsync against a seeded DuckDB fixture, plus a Darling.Tests source-census pin that the Lite reader's SQL names the same view and columns Darling's migrated schema and reader use. Ported the reader's column-name RED by temporarily renaming the FROM clause, confirming the new live pin fails, then reverting.
…and seed every required server_properties column in the Lite metadata test
…oints # Conflicts: # PerformanceMonitor.Ui/PlanViewerControl.xaml.cs
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 16:42
This was referenced Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4530
#4585 is merged; this is now against dev.
Part of #4511
What changes
Rule 38 (the Standard Edition batch-mode DOP-2 limitation) needs the server's edition and MAXDOP to give its Warning instead of an uninformative Info. This wires that metadata through every place a plan gets analyzed.
analyze_query_plan,analyze_procedure_plan,analyze_query_store_plan)DarlingServerMetadataReader.ReadAsync— one Postgres store read per callLocalDataService.GetServerMetadataForPlanAnalysisAsync— same shape, against DuckDBPgDrillDownCollector.Plans.cs,Lite/Analysis/DrillDownCollector.Plans.cs)ViewerServerTab.Plans.cs(Plan Viewer tab),ViewerActualPlanFlow.OpenFloatingPlanAsync(the shared floated-plan host, now takingdataService/serverId),ProcedureHistoryWindow,QueryStatsHistoryWindow,QueryStoreHistoryWindow,WaitDrillDownWindowViewerDataService.GetPlanAnalysisServerMetadataAsync(wrapsDarlingServerMetadataReader.ReadAsyncover the viewer's ownNpgsqlDataSource)ServerTab.Plans.cs(OpenPlanTab),FinOpsTab's High Impact/Expensive Queries menu,PlanViewerWindow.ShowPlanAsync/LoadPlanAsync(now take an optionalServerMetadata?),ProcedureHistoryWindow,QueryStatsHistoryWindow,QueryStoreHistoryWindow,WaitDrillDownWindowLocalDataService.GetServerMetadataForPlanAnalysisAsyncanalyze_plan_xmlnull, so rule 38 gives its Info branch, same as beforePlanAdvisoryAggregator.Summarize)null, per the earlier step's reasoningEvery viewer/Lite host that opens a
PlanViewerControl/PlanViewerWindowhas a known server id in scope (_server.ServerId,_serverId, or the currently-selected server), so every site could be wired; none were left null for lack of an id.Why a store read and not a live fetch. These entry points analyze stored plans with no live connection to the target server (the MCP tools and drill-downs read collected data; the viewer and Lite hosts have no target connection either). A stored read costs one indexed lookup per call —
server_propertiesanchored on(server_id, collection_time)for the newest row,server_config/v_server_configfor the newestmax degree of parallelismvalue — never cached, never per-plan-node.New shared-layer overloads (in
PerformanceMonitor.PlanAnalysis, used by every call site above):McpPlanAnalysisFormatter.BuildAnalysisResult(xml, serverName, source, identifier, ServerMetadata?, ct)— the existing 5-arg form forwardsnull.PlanAdvisoryAggregator.ExtractCancellable(planXmls, ServerMetadata?, ct)— the existing 2-arg form forwardsnull.Rule 38 itself (the analyzer logic, masking, message text) was already ported and pinned by an earlier step in this stack; this PR is entry-point wiring.
This push adds the reader-level pins the earlier wiring steps skipped. The prior test plan (
PlanSync4530EntryPointsTests) only ever fed a hand-builtServerMetadatainto the analyzer — it never proved thatDarlingServerMetadataReaderor Lite'sGetServerMetadataForPlanAnalysisAsyncactually READ the edition and MAXDOP off a real store. A wrong table or column name in either reader's SQL would returnnullsilently, and rule 38 would stay on its Info branch on every real install with nothing here to catch it. Three new pins close that gap:Darling/Darling.Tests/PlanSync4530ReaderLiveTests.cs— seedscollect.server_properties(edition "Standard Edition (64-bit)") andcollect.server_config(max degree of parallelism= 8) on a migrated scratch store, assertsDarlingServerMetadataReader.ReadAsyncreturns them, then runs the same DOP-2 batch-mode plan XML throughMcpPlanAnalysisFormatter.BuildAnalysisResult— the product's own MCP-tool call path — and confirms rule 38 gives its Warning; a server with no rows returnsnulland rule 38 stays Info. Also a source-census pin that Lite's reader SQL names the same view (v_server_properties/v_server_config) and columns (edition,max degree of parallelism,value_in_use) as Darling's reader.Darling/Darling.Tests/PlanSync4530DrillDownLiveTests.cs— seeds the same Standard/MAXDOP-8 server plus a query-plan row, runsPgDrillDownCollector.EnrichFindingsAsyncon aPLAN_WARNINGfinding, and asserts the drill-down'splan_warningscarries the rule-38 Warning — proving the drill-down's own store read (CollectPlanAdvisoryDetail→DarlingServerMetadataReader.ReadAsync→PlanAdvisoryAggregator.ExtractCancellable), not just the shared formatter.Lite.Tests/PlanServerMetadataReaderTests.cs— a build-only (net10.0-windows; CI runs it) pin forLocalDataService.GetServerMetadataForPlanAnalysisAsyncagainst a seeded DuckDB fixture (copied setup fromSharedDuckDbFixture/AnalysisAsOfAnchorTests's pattern): edition + MAXDOP round-trip, and a no-rows case returnsnull.Test plan
Darling/Darling.Tests/PlanSync4530EntryPointsTests.cs(landed earlier, unchanged): pins the shared-layerBuildAnalysisResult/ExtractCancellablemetadata overloads with a Warning vs. Info contrast on the same plan XML.PlanSync4530ReaderLiveTests,PlanSync4530DrillDownLiveTests,PlanSync4530Tests,PlanSync4530EntryPointsTests— Total: 15, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0.PlanSync4530ReaderLiveTests: renamed the reader'sFROM server_propertiesclause to a nonexistent table, rebuilt, re-ran — the new live pin failed (the read returnsnull, the assertion onmetadata.Editionfails). Reverted, rebuilt clean.DailyDeadlockWindowCensusTests,EntraProviderPackageCensusTests,FileGrowthRiseUnitCensusTests,McpPayloadContractCensusTests,McpServiceParameterDiSeatCensusTests,MeasurementContractCensusTests,MigrationDataMovingRungCensusPins,PgSettingScrubCandidateCensusTests,PlanForceActionDetailCensus(Tests),PlanSync4535RuleNumberCensusTests,RemoteCollectorServiceCancellationCensusTests,SameStatementPileupSourceCensusTests,StoreApplicationNameCensusTests,StoreSessionTimeZonePinCensusTests,WebExceptionTextCensusTests,McpToolsListBudgetTests).CommentFilterAdoptionTests,DocCommentHygieneTests,LiveCleanupConversionRatchetTestsalso run clean;LivePostgresCollectionHygieneTestshas one pre-existing failure unrelated to this change —EveryClassUsingTheSharedStore_IsSerializedOrDocumentsWhyNotthrowsReflectionTypeLoadExceptiononPresentationFramework(a WPF-type platform limitation on macOS, not this diff).-p:EnableWindowsTargeting=true), 0 errors / 0 warnings:Darling.Tests,Darling/PerformanceMonitor.Darling.Service,Lite.Tests.Lite.Tests.PlanServerMetadataReaderTestsbuilds clean (0 errors, 0 warnings) but does not run here (net10.0-windows discovery limitation on macOS); CI decides it. Read against the code: its setup copiesAnalysisAsOfAnchorTests'sSharedDuckDbFixturepattern exactly (constructorResetData,AcquireReadLockaround every insert,IDisposablecleanup of the seed connection) — the pattern the WINDOWS-ONLY PINS rule calls out as CI-safe.DarlingMcpPlanToolsLivePostgresTestswas not run (needs the sharedlive-postgrescollection fixture beyond this container's ad-hoc setup); it is unaffected by this push and, as corrected below, was never claimed to cover the metadata wiring — only the tool surface and fetch/analyze round-trips.CHANGELOG
SECTION: Changed
ENTRY: - Rule 38 can now flag a Standard Edition DOP limit as a Warning ([#4601]) - Plan analysis reads each server's collected edition and MAXDOP wherever it analyzes a stored plan (the MCP plan tools, the drill-downs, and the Darling Viewer and Lite plan viewers). A plan that ran at DOP 2 with batch-mode operators on a Standard Edition server with MAXDOP above 2 now gets a Warning; it stays Info when the edition isn't known.
REF: [#4601]: #4601
scrubcheck: clean.