From 334dbffc7d2c577a174e116293cd8bb0dfde51ab Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 27 Sep 2026 21:50:51 -0400 Subject: [PATCH] Say which plan warnings are SQL Server's and which are the analyzer's, and key duplicate index suggestions on the database (#4520) (#4523) --- Darling/Darling.Tests/PlanSync4520Tests.cs | 87 +++++++++++++++++++ Darling/Darling.Tests/PlanSync4523Tests.cs | 65 ++++++++++++++ .../McpPlanAnalysisFormatter.cs | 3 +- .../PlanAnalyzer.cs | 4 +- PerformanceMonitor.PlanAnalysis/PlanModels.cs | 21 +++++ .../ShowPlanParser.cs | 6 ++ .../PlanViewerControl.Properties.cs | 12 ++- 7 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 Darling/Darling.Tests/PlanSync4520Tests.cs create mode 100644 Darling/Darling.Tests/PlanSync4523Tests.cs diff --git a/Darling/Darling.Tests/PlanSync4520Tests.cs b/Darling/Darling.Tests/PlanSync4520Tests.cs new file mode 100644 index 0000000000..bb7fc7c2dd --- /dev/null +++ b/Darling/Darling.Tests/PlanSync4520Tests.cs @@ -0,0 +1,87 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System.Linq; +using PerformanceMonitor.PlanAnalysis; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4520 (part of #4511, ported from erikdarlingdata/PerformanceStudio@60eb0bf): a reader could not tell a +/// warning SQL Server itself wrote into the plan apart from one the analyzer inferred from plan shape, because +/// both arrive as a carrying nothing but type, severity and message. +/// +/// defaults to . Everything +/// ShowPlanParser.ParseWarningsFromElement returns is stamped +/// in one place, so a warning lifted straight out of the plan's own <Warnings> element is marked as +/// the engine's record, not an inference the analyzer could get wrong for a given plan. +/// +/// The repro XML is synthetic (dbo.t, [db]): one Table Scan carries both a +/// PlanAffectingConvert warning (the engine's own record) and a non-SARGable residual predicate that Rule +/// 12 flags as "Non-SARGable Predicate" (the analyzer's inference). +/// +public sealed class PlanSync4520Tests +{ + private const string ReproXml = """ + + + + + + + + + + + + + + + + + + """; + + private static ParsedPlan ParseAndAnalyze() + { + var plan = ShowPlanParser.Parse(ReproXml); + PlanAnalyzer.Analyze(plan); + return plan; + } + + [Fact] + public void EngineWarning_IsStampedSqlServer() + { + var plan = ParseAndAnalyze(); + var scanNode = plan.Batches.Single().Statements.Single().RootNode!.Children.Single(); + var convertWarning = scanNode.Warnings.Single(w => w.WarningType == "Implicit Conversion"); + + Assert.Equal(PlanWarningSource.SqlServer, convertWarning.Source); + } + + [Fact] + public void AnalyzerWarning_KeepsTheDefaultSource() + { + var plan = ParseAndAnalyze(); + var scanNode = plan.Batches.Single().Statements.Single().RootNode!.Children.Single(); + var sargableWarning = scanNode.Warnings.Single(w => w.WarningType == "Non-SARGable Predicate"); + + Assert.Equal(PlanWarningSource.Analyzer, sargableWarning.Source); + } + + [Fact] + public void McpFormatter_CarriesSourceOnEachWarning() + { + // Goes through the product's own MCP formatting path, not the parser directly. + var json = McpPlanAnalysisFormatter.BuildAnalysisResult(ReproXml, serverName: null, source: "test", identifier: null); + + Assert.Contains("\"source\":\"SqlServer\"", json); + Assert.Contains("\"source\":\"Analyzer\"", json); + } +} diff --git a/Darling/Darling.Tests/PlanSync4523Tests.cs b/Darling/Darling.Tests/PlanSync4523Tests.cs new file mode 100644 index 0000000000..be0918f4ac --- /dev/null +++ b/Darling/Darling.Tests/PlanSync4523Tests.cs @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System.Linq; +using PerformanceMonitor.PlanAnalysis; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4523 (part of #4511, ported from erikdarlingdata/PerformanceStudio@cc18844): Rule 30's duplicate-index- +/// suggestion check grouped missing index suggestions by $"{Schema}.{Table}", so a plan touching a +/// same-named table (dbo.t) in two different databases had its suggestions merged and reported as +/// duplicates of each other, even though they target unrelated tables. already +/// carries Database; the fix adds it to the grouping key. +/// +/// The repro XML is synthetic (dbo.t, [db1]/[db2]): one statement with two missing +/// index groups for [db1].[dbo].[t] and [db2].[dbo].[t]. +/// +public sealed class PlanSync4523Tests +{ + private static string ReproXml(string db1, string db2) => $""" + + + + + + + + + + + + """; + + private static ParsedPlan ParseAndAnalyze(string db1, string db2) + { + var plan = ShowPlanParser.Parse(ReproXml(db1, db2)); + PlanAnalyzer.Analyze(plan); + return plan; + } + + [Fact] + public void SameTableName_DifferentDatabases_AreNotReportedAsDuplicates() + { + var plan = ParseAndAnalyze("db1", "db2"); + var stmt = plan.Batches.Single().Statements.Single(); + + Assert.DoesNotContain(stmt.PlanWarnings, w => w.WarningType == "Duplicate Index Suggestions"); + } + + [Fact] + public void SameTableName_SameDatabase_AreStillReportedAsDuplicates() + { + var plan = ParseAndAnalyze("db1", "db1"); + var stmt = plan.Batches.Single().Statements.Single(); + + Assert.Contains(stmt.PlanWarnings, w => w.WarningType == "Duplicate Index Suggestions"); + } +} diff --git a/PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs b/PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs index a896a6fd75..17be3fe13d 100644 --- a/PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs +++ b/PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs @@ -154,7 +154,8 @@ RankedByActualElapsed. truncated is OBSERVED against the whole population (every { severity = w.Severity.ToString(), type = w.WarningType, - message = w.Message + message = w.Message, + source = w.Source.ToString() }), warning_count = allWarnings.Count, critical_count = allWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical), diff --git a/PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs b/PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs index 2031492e2d..ed31fcc934 100644 --- a/PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs +++ b/PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs @@ -336,7 +336,7 @@ private static void AnalyzeStatement(PlanStatement stmt) { // Detect duplicate suggestions for the same table var tableSuggestionCount = stmt.MissingIndexes - .GroupBy(mi => $"{mi.Schema}.{mi.Table}", StringComparer.OrdinalIgnoreCase) + .GroupBy(mi => $"{mi.Database}.{mi.Schema}.{mi.Table}", StringComparer.OrdinalIgnoreCase) .Where(g => g.Count() > 1) .ToDictionary(g => g.Key, g => g.Count(), StringComparer.OrdinalIgnoreCase); @@ -344,7 +344,7 @@ private static void AnalyzeStatement(PlanStatement stmt) { var keyCount = mi.EqualityColumns.Count + mi.InequalityColumns.Count; var includeCount = mi.IncludeColumns.Count; - var tableKey = $"{mi.Schema}.{mi.Table}"; + var tableKey = $"{mi.Database}.{mi.Schema}.{mi.Table}"; // Low-impact suggestion (< 25% improvement) if (mi.Impact < 25) diff --git a/PerformanceMonitor.PlanAnalysis/PlanModels.cs b/PerformanceMonitor.PlanAnalysis/PlanModels.cs index ffab64d5d7..8eaea00e46 100644 --- a/PerformanceMonitor.PlanAnalysis/PlanModels.cs +++ b/PerformanceMonitor.PlanAnalysis/PlanModels.cs @@ -394,6 +394,17 @@ public class PlanWarning public PlanWarningSeverity Severity { get; set; } public SpillDetail? SpillDetails { get; set; } + /// + /// Who says so — SQL Server itself, or the analyzer. Defaults to + /// because the analyzer builds the large majority of warnings. Everything the parser lifts out of + /// the plan's own <Warnings> element is stamped in one + /// place, at the single return of ShowPlanParser.ParseWarningsFromElement, so a new engine warning + /// cannot be added and forgotten. A warning the engine wrote into the plan is a record of what + /// happened when the query ran; an analyzer rule is an inference from plan shape, and an inference + /// can be wrong about a particular plan in a way the engine's own record cannot be. + /// + public PlanWarningSource Source { get; set; } = PlanWarningSource.Analyzer; + /// /// Maximum percentage of elapsed time that could be saved by addressing this finding. /// null = not quantifiable, 0 = calculated as negligible. @@ -408,6 +419,16 @@ public class PlanWarning public enum PlanWarningSeverity { Info, Warning, Critical } +/// Where a came from. See . +public enum PlanWarningSource +{ + /// An inference of the analyzer's, from the shape of the plan. + Analyzer, + + /// Read out of the plan's own <Warnings> element — the engine's record, not the analyzer's. + SqlServer +} + public class MemoryGrantInfo { public long SerialRequiredMemoryKB { get; set; } diff --git a/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs b/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs index 56ce8c164c..db3cd4573a 100644 --- a/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs +++ b/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs @@ -1764,6 +1764,12 @@ private static List ParseWarningsFromElement(XElement warningsEl) }); } + /* Stamped here rather than on each construction above, so that everything read out of the + plan's own element is marked as the engine's, including whatever gets added to + this method next. This is the only place parser warnings are built. */ + foreach (var warning in result) + warning.Source = PlanWarningSource.SqlServer; + return result; } diff --git a/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs b/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs index 7ecf356764..0040d9f190 100644 --- a/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs +++ b/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs @@ -818,7 +818,7 @@ private void ShowPropertiesPanel(PlanNode node) var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; warnPanel.Children.Add(new TextBlock { - Text = $"\u26A0 {w.WarningType}", + Text = $"\u26A0 {w.WarningType}{WarningSourceTag(w)}", FontWeight = FontWeights.SemiBold, FontSize = 11, Foreground = new SolidColorBrush((Color)ColorConverter.ConvertFromString(warnColor)) @@ -859,7 +859,7 @@ private void ShowPropertiesPanel(PlanNode node) var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; warnPanel.Children.Add(new TextBlock { - Text = $"\u26A0 {w.WarningType}", + Text = $"\u26A0 {w.WarningType}{WarningSourceTag(w)}", FontWeight = FontWeights.SemiBold, FontSize = 11, Foreground = new SolidColorBrush((Color)ColorConverter.ConvertFromString(warnColor)) @@ -1117,6 +1117,14 @@ private void ShowWaitStats(List waits, bool isActualPlan) // Wait category + color come from the shared ChartPalette (PerformanceStudio's ~21-category // taxonomy + palette), with a light-theme override for the near-invisible pale colors (D3). + /// + /// #4520: marks the warnings SQL Server itself wrote into the plan, so they are not read as one of + /// the analyzer's inferences. Only the engine's are tagged — they are the minority, and a badge on + /// every warning would carry no information. + /// + private static string WarningSourceTag(PlanWarning warning) => + warning.Source == PlanWarningSource.SqlServer ? " [SQL Server]" : ""; + private static string GetWaitCategory(string waitType) => ChartPalette.WaitCategory(waitType); private static string GetWaitCategoryColor(string category)