diff --git a/CITATION.cff b/CITATION.cff index c3281a5..5bcbb8d 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,7 +9,7 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.20.0" +version: "1.21.0" date-released: "2026-08-21" keywords: - sql-server diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 52a638c..3142b1c 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.20.0 + 1.21.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs index a67609e..7dd13b2 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs @@ -46,6 +46,49 @@ private void SelectNode(Border border, PlanNode node) UpdateMinimapSelection(node); } + /// + /// Selects the operator with and scrolls it into view, so a warning + /// can take you to where it came from (#440). Returns false when the plan has no such operator, + /// which is what keeps a stale or wrong origin from silently scrolling somewhere arbitrary. + /// + private bool TryNavigateToNode(int nodeId) + { + foreach (var child in PlanCanvas.Children) + { + if (child is not Border border || + !_nodeBorderMap.TryGetValue(border, out var node) || + node.NodeId != nodeId) + { + continue; + } + + SelectNode(border, node); + ScrollNodeIntoView(node); + return true; + } + + return false; + } + + /// + /// Centres the operator in the viewport. Node coordinates are unscaled layout positions, so they + /// are multiplied by the zoom level to get canvas pixels; the offset is then clamped, because + /// asking a ScrollViewer for a negative offset on a plan smaller than the viewport just leaves + /// it where it was and looks like the navigation did nothing. + /// + private void ScrollNodeIntoView(PlanNode node) + { + var targetX = node.X * _zoomLevel - (PlanScrollViewer.Bounds.Width / 2); + var targetY = node.Y * _zoomLevel - (PlanScrollViewer.Bounds.Height / 2); + + var maxX = Math.Max(0, PlanScrollViewer.Extent.Width - PlanScrollViewer.Viewport.Width); + var maxY = Math.Max(0, PlanScrollViewer.Extent.Height - PlanScrollViewer.Viewport.Height); + + PlanScrollViewer.Offset = new Vector( + Math.Clamp(targetX, 0, maxX), + Math.Clamp(targetY, 0, maxY)); + } + private ContextMenu BuildNodeContextMenu(PlanNode node) { var menu = new ContextMenu(); diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index dbd8369..500514c 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -822,13 +822,15 @@ private void ShowPropertiesPanel(PlanNode node) var planWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : $"\u26A0 {w.WarningType}{sourceTag}{legacyTag}"; - warnPanel.Children.Add(new TextBlock + var planWarnHeaderBlock = new TextBlock { Text = planWarnHeader, FontWeight = FontWeight.SemiBold, FontSize = 11, Foreground = new SolidColorBrush(Color.Parse(warnColor)) - }); + }; + AttachOriginNavigation(planWarnHeaderBlock, planWarnHeader, w.OriginNodeIds); + warnPanel.Children.Add(planWarnHeaderBlock); warnPanel.Children.Add(new TextBlock { Text = w.Message, @@ -875,6 +877,78 @@ private void ShowPropertiesPanel(PlanNode node) PropertiesContent.Children.Add(planWarningsExpander); } + /* === Operator Warnings (#440) === + Every warning hanging off an operator anywhere in this statement, gathered in one + place and each one a link to its operator. + + This section is the point of #440. The reporter's case was "the plan is huge and the + warning origin is murky", and the operator warnings are exactly the ones with a + murky origin - but until now the only way to see one was to already have clicked the + operator it was on, which is no help when you do not know which operator to click. + Nothing is removed from the per-operator panel; this is an index into it. */ + var operatorWarnings = WarningIndex.CollectOperatorWarnings(s.RootNode); + if (operatorWarnings.Count > 0) + { + var operatorWarningsPanel = new StackPanel(); + foreach (var (originNode, w) in operatorWarnings + .OrderByDescending(x => x.Warning.MaxBenefitPercent ?? -1) + .ThenByDescending(x => x.Warning.Severity) + .ThenBy(x => x.Warning.WarningType)) + { + var opWarnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373" + : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; + var opWarnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; + var opBenefit = w.MaxBenefitPercent.HasValue + ? $" \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : ""; + var opHeaderText = + $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{opBenefit}"; + var opHeader = new TextBlock + { + Text = opHeaderText, + FontWeight = FontWeight.SemiBold, + FontSize = 11, + Foreground = new SolidColorBrush(Color.Parse(opWarnColor)) + }; + AttachOriginNavigation(opHeader, opHeaderText, w.OriginNodeIds); + opWarnPanel.Children.Add(opHeader); + opWarnPanel.Children.Add(new TextBlock + { + Text = OperatorOriginLabel(originNode), + FontSize = 11, + Foreground = SectionHeaderBrush, + Margin = new Thickness(16, 0, 0, 0) + }); + operatorWarningsPanel.Children.Add(opWarnPanel); + } + + var operatorWarningsExpander = new Expander + { + /* Collapsed by default, unlike Plan Warnings. On a large plan this is the + longest section in the panel, and expanding it by default would push the + statement's own details off screen - the opposite of the problem #440 is + about. */ + IsExpanded = false, + Header = new TextBlock + { + Text = $"Operator Warnings ({operatorWarnings.Count})", + FontWeight = FontWeight.SemiBold, + FontSize = 11, + Foreground = SectionHeaderBrush + }, + Content = operatorWarningsPanel, + Margin = new Thickness(0, 2, 0, 0), + Padding = new Thickness(0), + Foreground = SectionHeaderBrush, + Background = new SolidColorBrush(Color.FromArgb(0x18, 0x4F, 0xA3, 0xFF)), + BorderBrush = PropSeparatorBrush, + BorderThickness = new Thickness(0, 0, 0, 1), + HorizontalAlignment = HorizontalAlignment.Stretch, + HorizontalContentAlignment = HorizontalAlignment.Stretch + }; + PropertiesContent.Children.Add(operatorWarningsExpander); + } + // === Missing Indexes === if (s.MissingIndexes.Count > 0) { diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index ae4ef22..ca337fe 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -516,6 +516,46 @@ private static string FormatBytes(double bytes) private static string FormatBenefitPercent(double pct) => pct >= 100 ? $"{pct:N0}" : $"{pct:N1}"; + /// + /// How an operator is named in the aggregated warning index (#440) — enough to recognise it + /// before clicking, matching what the operator's own panel puts in its header. + /// + private static string OperatorOriginLabel(PlanNode node) => + string.IsNullOrEmpty(node.FullObjectName) + ? $"Node {node.NodeId} \u00B7 {node.PhysicalOp}" + : $"Node {node.NodeId} \u00B7 {node.PhysicalOp} on {node.FullObjectName}"; + + /// + /// Turns a warning header into a link to the operator it came from (#440). + /// + /// Only when the warning actually knows. Findings with no operator origin — "High Compile + /// CPU" happened before a row was read — are left as plain text rather than given a link that + /// goes somewhere arbitrary, because a reader would believe it. + /// + /// Where a warning came from several operators, the first is the navigation target and the + /// rest are named in the tooltip, so the count is visible rather than silently dropped. + /// + private void AttachOriginNavigation(TextBlock header, string headerText, List originNodeIds) + { + if (originNodeIds.Count == 0) + return; + + header.Text = headerText + " \u2192"; + header.Cursor = new Cursor(StandardCursorType.Hand); + ToolTip.SetTip(header, originNodeIds.Count == 1 + ? $"Go to operator (Node {originNodeIds[0]})" + : $"Go to Node {originNodeIds[0]} — also from {string.Join(", ", originNodeIds.Skip(1).Select(id => "Node " + id))}"); + + header.PointerPressed += (_, e) => + { + if (!e.GetCurrentPoint(header).Properties.IsLeftButtonPressed) + return; + + if (TryNavigateToNode(originNodeIds[0])) + e.Handled = true; + }; + } + /// /// #436: marks the warnings SQL Server itself wrote into the plan, so they are not read as one of /// our inferences. Only the engine's are tagged — they are the minority, and a badge on every diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs index 9a612cc..a1db698 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs @@ -315,6 +315,11 @@ private void SetStatus(string text, bool autoClear = true) StatusText.Text = text; + /* The bar is one line and trims with an ellipsis, so a long message - an error, usually - + is readable only on hover. Setting the tip to the same text costs nothing when it fits and + is the difference between a truncated error and a recoverable one when it does not. */ + ToolTip.SetTip(StatusText, string.IsNullOrEmpty(text) ? null : text); + if (autoClear && !string.IsNullOrEmpty(text)) { var cts = new CancellationTokenSource(); diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs index 4145b51..9f9582a 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs @@ -83,12 +83,17 @@ private async Task CaptureAndShowPlan(bool estimated, string? queryTextOverride Margin = new Avalonia.Thickness(0, 0, 0, 12) }; - var statusLabel = new TextBlock + /* #448: SelectableTextBlock and wrapping, because this label doubles as the place a query + failure is reported. A SQL error is the one string in this app a user most needs to copy + somewhere else, and unwrapped it was being clipped by the panel. */ + var statusLabel = new SelectableTextBlock { Text = $"Capturing {planType.ToLower()} plan...", FontSize = 14, Foreground = new SolidColorBrush(Color.Parse("#E4E6EB")), - HorizontalAlignment = HorizontalAlignment.Center + HorizontalAlignment = HorizontalAlignment.Center, + TextAlignment = Avalonia.Media.TextAlignment.Center, + TextWrapping = TextWrapping.Wrap }; var cancelBtn = new Button @@ -211,18 +216,44 @@ private async Task CaptureAndShowPlan(bool estimated, string? queryTextOverride } catch (SqlException ex) { - statusLabel.Text = ex.Message.Length > 100 ? ex.Message[..100] + "..." : ex.Message; - progressBar.IsVisible = false; - cancelBtn.IsVisible = false; + ShowExecutionFailure(loadingPanel, statusLabel, progressBar, cancelBtn, ex.Message); } catch (Exception ex) { - statusLabel.Text = ex.Message.Length > 100 ? ex.Message[..100] + "..." : ex.Message; - progressBar.IsVisible = false; - cancelBtn.IsVisible = false; + ShowExecutionFailure(loadingPanel, statusLabel, progressBar, cancelBtn, ex.Message); } } + /// + /// Reports a query failure in the plan tab, in full (#448). + /// + /// It used to be cut to 100 characters with an ellipsis, in a panel fixed at 300px wide + /// holding a non-wrapping label — three separate reasons the same message got clipped, and + /// between them a SQL error was routinely unreadable. 100 characters does not even reach the end + /// of "Msg 208, Level 16, State 1, Procedure X, Line N" before the sentence naming the actual + /// problem starts. + /// + /// The panel is sized for a spinner and a Cancel button, which is why it is narrow; on + /// failure it is re-sized for prose. MaxWidth rather than Width, so a short error stays compact + /// and a long one is bounded at a readable measure instead of running the width of the window. + /// + internal static void ShowExecutionFailure( + StackPanel panel, + SelectableTextBlock statusLabel, + ProgressBar progressBar, + Button cancelBtn, + string message) + { + panel.Width = double.NaN; + panel.MaxWidth = 640; + + statusLabel.Text = message; + statusLabel.Foreground = new SolidColorBrush(Color.Parse("#E57373")); + + progressBar.IsVisible = false; + cancelBtn.IsVisible = false; + } + private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) { var viewer = GetSelectedPlanViewer(); @@ -274,12 +305,15 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) Margin = new Avalonia.Thickness(0, 0, 0, 12) }; - var statusLabel = new TextBlock + /* #448: see the note on the estimated-plan path — this label reports failures too. */ + var statusLabel = new SelectableTextBlock { Text = "Capturing actual plan...", FontSize = 14, Foreground = new SolidColorBrush(Color.Parse("#E4E6EB")), - HorizontalAlignment = HorizontalAlignment.Center + HorizontalAlignment = HorizontalAlignment.Center, + TextAlignment = Avalonia.Media.TextAlignment.Center, + TextWrapping = TextWrapping.Wrap }; var cancelBtn = new Button @@ -385,15 +419,11 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) } catch (SqlException ex) { - statusLabel.Text = ex.Message.Length > 100 ? ex.Message[..100] + "..." : ex.Message; - progressBar.IsVisible = false; - cancelBtn.IsVisible = false; + ShowExecutionFailure(loadingPanel, statusLabel, progressBar, cancelBtn, ex.Message); } catch (Exception ex) { - statusLabel.Text = ex.Message.Length > 100 ? ex.Message[..100] + "..." : ex.Message; - progressBar.IsVisible = false; - cancelBtn.IsVisible = false; + ShowExecutionFailure(loadingPanel, statusLabel, progressBar, cancelBtn, ex.Message); } finally { diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs index b343fdf..f230526 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs @@ -220,7 +220,30 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) } } + /// + /// #447: asks the window, not this session. Comparing the plan from one query against the plan + /// from another is the ordinary case, and counting only this session's own tabs left the button + /// disabled in both — the reporter had to save a plan and reopen it to get at a comparison the + /// app could already do. + /// private void UpdateCompareButtonState() + { + if (TopLevel.GetTopLevel(this) is MainWindow owner) + { + /* Refreshes every session, not just this one: a plan appearing here can be the second + plan that makes Compare available over THERE. */ + owner.RefreshComparePlanAvailability(); + return; + } + + /* No owning window — the control is being hosted somewhere else or is not attached yet. + Fall back to what this session can see rather than leaving the button in a stale state. */ + SetCompareAvailability(CountOwnPlans() >= 2); + } + + internal void SetCompareAvailability(bool enabled) => ComparePlansButton.IsEnabled = enabled; + + private int CountOwnPlans() { int planCount = 0; foreach (var item in SubTabControl.Items) @@ -228,7 +251,7 @@ private void UpdateCompareButtonState() if (item is TabItem t && t.Content is PlanViewerControl v && v.CurrentPlan != null) planCount++; } - ComparePlansButton.IsEnabled = planCount >= 2; + return planCount; } private static string GetTabLabel(TabItem tab) @@ -242,10 +265,19 @@ private static string GetTabLabel(TabItem tab) private void ComparePlans_Click(object? sender, RoutedEventArgs e) { + /* #447: hand off to the window's picker, which lists plans from every session and labels + them "Query 1 > Plan". This session's own picker cannot see the other query's plan, which + is the whole complaint. */ + if (TopLevel.GetTopLevel(this) is MainWindow owner) + { + owner.ShowCompareDialog(); + return; + } + var planTabs = GetPlanTabs().ToList(); if (planTabs.Count < 2) { - SetStatus("Need at least 2 plan tabs to compare"); + SetStatus("Need at least 2 plans open to compare"); return; } diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs index ca308f8..fe2bf9b 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs @@ -216,7 +216,12 @@ private async void QueryStore_Click(object? sender, RoutedEventArgs e) } catch (Exception ex) { - SetStatus(ex.Message.Length > 80 ? ex.Message[..80] + "..." : ex.Message, autoClear: false); + /* Was cut to 80 characters here before being handed to a status bar that already does + TextTrimming="CharacterEllipsis". The trimming is the bar's job and it does it against + the actual available width; doing it again in code just threw away text the control + would have kept, and with it the tooltip that now carries the full message. Same + family as #448. */ + SetStatus(ex.Message, autoClear: false); return; } diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml b/src/PlanViewer.App/Controls/QuerySessionControl.axaml index f651f35..0c40f54 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml @@ -61,7 +61,7 @@ Click="ComparePlans_Click" Height="28" Padding="8,0" FontSize="12" IsEnabled="False" Theme="{StaticResource AppButton}" - ToolTip.Tip="Compare two plan tabs"/> + ToolTip.Tip="Compare any two plans open in this window"/> public PlanWarningSource Source { get; set; } = PlanWarningSource.PerformanceStudio; + /// + /// The operators this finding actually came from, so a reader can be taken to them (#440). + /// + /// A LIST rather than a single id, because the three honest answers are genuinely + /// different. A key lookup came from exactly one operator. A table variable warning came from + /// every operator that touched one, which on a big plan is several. And some findings have no + /// operator at all — "High Compile CPU" happened before a single row was read, and + /// "UDF Execution" is reported by SQL Server at the statement level only. Those keep this + /// empty, and the UI offers no navigation rather than picking somewhere arbitrary. + /// + /// Sending a reader to the wrong operator is worse than sending them nowhere, because + /// they would believe it. + /// + public List OriginNodeIds { get; set; } = []; + /// /// Maximum percentage of elapsed time that could be saved by addressing this finding. /// null = not quantifiable, 0 = calculated as negligible. diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index 267f4fc..c9a3e70 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -256,6 +256,15 @@ public class WarningResult /// [JsonPropertyName("source")] public string Source { get; set; } = ""; + + /// + /// Node ids of the operators this finding came from, empty when it has no operator origin — + /// "High Compile CPU" happened before any operator ran (#440). Distinct from + /// , which says where the warning is ATTACHED in this output; these say + /// where it came FROM, and for a statement-level warning there is no NodeId at all. + /// + [JsonPropertyName("origin_node_ids")] + public List OriginNodeIds { get; set; } = []; } public class MissingIndexResult diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 97996af..83ac5fd 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -196,7 +196,8 @@ private static StatementResult MapStatement( MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, IsLegacy = w.IsLegacy, - Source = w.Source.ToString() + Source = w.Source.ToString(), + OriginNodeIds = w.OriginNodeIds }); } @@ -306,7 +307,8 @@ private static OperatorResult MapNode(PlanNode node, CancellationToken cancellat MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, IsLegacy = w.IsLegacy, - Source = w.Source.ToString() + Source = w.Source.ToString(), + OriginNodeIds = w.OriginNodeIds }); } diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs index 29086f6..ce63fa3 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs @@ -21,12 +21,18 @@ private static bool HasBatchModeNode(PlanNode node) return false; } + /* #440: collects the operators it found, because this walk already knows exactly which ones + touched a table variable and used to throw that away. Two lists rather than one, since the + two warnings this feeds are about different operators: every operator referencing a table + variable, versus only the ones modifying it (which is what forces the plan serial). */ private static void CheckForTableVariables(PlanNode node, bool isModification, - ref bool hasTableVar, ref bool modifiesTableVar) + ref bool hasTableVar, ref bool modifiesTableVar, + List? referencingNodeIds = null, List? modifyingNodeIds = null) { if (!string.IsNullOrEmpty(node.ObjectName) && node.ObjectName.StartsWith("@")) { hasTableVar = true; + referencingNodeIds?.Add(node.NodeId); // The modification target is typically an Insert/Update/Delete operator on a table variable if (isModification && (node.PhysicalOp.Contains("Insert", StringComparison.OrdinalIgnoreCase) || node.PhysicalOp.Contains("Update", StringComparison.OrdinalIgnoreCase) @@ -34,10 +40,12 @@ private static void CheckForTableVariables(PlanNode node, bool isModification, || node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase))) { modifiesTableVar = true; + modifyingNodeIds?.Add(node.NodeId); } } foreach (var child in node.Children) - CheckForTableVariables(child, isModification, ref hasTableVar, ref modifiesTableVar); + CheckForTableVariables(child, isModification, ref hasTableVar, ref modifiesTableVar, + referencingNodeIds, modifyingNodeIds); } /// diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs index 2a702bf..8dcbbc2 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs @@ -48,6 +48,20 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt, AnalyzerConfi Rule28_RowCountSpool(node, stmt, cfg); Rule29_ImplicitConversionSeek(node, stmt, cfg); Rule35_ExpensiveOperator(node, stmt, cfg); + + /* #440: an operator warning's origin is the operator it is hanging off, so it is stamped + here rather than at each of the 26 places above that add one. Same reasoning as the + provenance stamp in ShowPlanParser: a rule you have to remember at every construction + site is a rule that eventually gets forgotten, and the one time it is forgotten the UI + quietly offers no link on a warning that has a perfectly good one. + + Only fills what a rule left empty, so a rule that knows better - one pointing at the + operator that CAUSED the problem rather than the one reporting it - keeps its own answer. */ + foreach (var warning in node.Warnings) + { + if (warning.OriginNodeIds.Count == 0) + warning.OriginNodeIds.Add(node.NodeId); + } } private static void Rule01_FilterOperator(PlanNode node, PlanStatement stmt, AnalyzerConfig cfg) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs index 901e5ee..bce1d0c 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs @@ -488,7 +488,10 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig var hasTableVar = false; var isModification = stmt.StatementType is "INSERT" or "UPDATE" or "DELETE" or "MERGE"; var modifiesTableVar = false; - CheckForTableVariables(stmt.RootNode, isModification, ref hasTableVar, ref modifiesTableVar); + var referencingNodeIds = new List(); + var modifyingNodeIds = new List(); + CheckForTableVariables(stmt.RootNode, isModification, ref hasTableVar, ref modifiesTableVar, + referencingNodeIds, modifyingNodeIds); if (hasTableVar && !modifiesTableVar) { @@ -496,7 +499,8 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig { WarningType = "Table Variable", Message = "Table variable detected. Table variables lack column-level statistics, which causes bad row estimates, join choices, and memory grant decisions. Replace with a #temp table.", - Severity = PlanWarningSeverity.Warning + Severity = PlanWarningSeverity.Warning, + OriginNodeIds = referencingNodeIds }); } @@ -506,7 +510,8 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig { WarningType = "Table Variable", Message = "This query modifies a table variable, which forces the entire plan to run single-threaded. SQL Server cannot use parallelism for modifications to table variables. Replace with a #temp table to allow parallel execution.", - Severity = PlanWarningSeverity.Critical + Severity = PlanWarningSeverity.Critical, + OriginNodeIds = modifyingNodeIds }); } } diff --git a/src/PlanViewer.Core/Services/WarningIndex.cs b/src/PlanViewer.Core/Services/WarningIndex.cs new file mode 100644 index 0000000..5d4f8d6 --- /dev/null +++ b/src/PlanViewer.Core/Services/WarningIndex.cs @@ -0,0 +1,41 @@ +using System.Collections.Generic; +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// Gathers the warnings scattered across an operator tree into one list (#440). +/// +/// Lives in Core rather than next to the panel that renders it for two reasons: it is a plan +/// tree walk over Core's own models with nothing UI about it, and putting it here means it can be +/// tested without standing up Avalonia. +/// +public static class WarningIndex +{ + /// + /// Every warning hanging off an operator beneath , paired with the + /// operator carrying it, in no particular order — callers sort for themselves, because the + /// statement panel wants them by benefit while other callers may not. + /// + public static List<(PlanNode Node, PlanWarning Warning)> CollectOperatorWarnings(PlanNode? root) + { + var collected = new List<(PlanNode, PlanWarning)>(); + if (root == null) + return collected; + + /* Explicit stack rather than recursion: a deep plan is exactly the case this feature exists + for, and #430 was a crash caused by assuming operator trees are shallow. */ + var pending = new Stack(); + pending.Push(root); + while (pending.Count > 0) + { + var node = pending.Pop(); + foreach (var warning in node.Warnings) + collected.Add((node, warning)); + foreach (var child in node.Children) + pending.Push(child); + } + + return collected; + } +} diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index 6ff4ddb..0e55fb2 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.20.0.0")] -[assembly: AssemblyFileVersion("1.20.0.0")] +[assembly: AssemblyVersion("1.21.0.0")] +[assembly: AssemblyFileVersion("1.21.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index 191276b..0c2ef89 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS diff --git a/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs new file mode 100644 index 0000000..b8b70f9 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs @@ -0,0 +1,100 @@ +using System.IO; +using System.Linq; +using Avalonia.Controls; +using Avalonia.Interactivity; +using PlanViewer.App; +using PlanViewer.App.Controls; + +namespace PlanViewer.Core.Tests; + +/// +/// #447: Compare Plans stayed disabled when the two plans lived in two different query sessions, +/// which is the ordinary case — run a query, rewrite it, run it again in a second tab. The button's +/// enablement counted only the session's OWN plan tabs, while the picker behind it had always been +/// able to see across sessions. The reporter had to save a plan and reopen it to get at a comparison +/// the app could already do. +/// +/// The scenario is built from plan FILES rather than executed queries deliberately: getting a plan +/// into a session needs a live SQL Server, and the defect does not require one. What it requires is +/// plans existing somewhere OTHER than the session whose button is being judged, which two file tabs +/// provide exactly. +/// +public class ComparePlansAvailabilityTests +{ + [Fact] + public void ASessionOffersCompareWhenThePlansAreElsewhereInTheWindow() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + window.LoadPlanFile(PlanPath("key_lookup_plan.sqlplan")); + window.NewQuery_Click(window, new RoutedEventArgs()); + + /* None of these sessions hold plans of their own — precisely the state that used to + disable the button, and precisely the state the reporter was in. Asserted across every + session rather than one, because the fix has to reach all of them. */ + var sessions = Sessions(window).ToList(); + Assert.NotEmpty(sessions); + Assert.All(sessions, session => Assert.Empty(session.GetPlanTabs())); + Assert.All(sessions, session => Assert.True( + CompareButton(session).IsEnabled, + "two plans are open in this window, so every session should offer to compare them")); + }); + } + + [Fact] + public void ASingleOpenPlanIsStillNotEnoughToCompare() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + window.NewQuery_Click(window, new RoutedEventArgs()); + + Assert.All(Sessions(window), session => Assert.False( + CompareButton(session).IsEnabled, + "one plan cannot be compared against anything")); + }); + } + + /// + /// The part that needed a window-wide refresh rather than a per-session one: a plan opening in + /// one place has to light the button up everywhere else, including in sessions that existed + /// before it arrived. + /// + [Fact] + public void OpeningASecondPlanEnablesCompareInASessionThatAlreadyExisted() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.NewQuery_Click(window, new RoutedEventArgs()); + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + + var sessions = Sessions(window).ToList(); + Assert.NotEmpty(sessions); + Assert.All(sessions, session => Assert.False(CompareButton(session).IsEnabled)); + + window.LoadPlanFile(PlanPath("key_lookup_plan.sqlplan")); + + Assert.All(sessions, session => Assert.True(CompareButton(session).IsEnabled, + "the session existed before the second plan did, and must still notice it")); + }); + } + + private static string PlanPath(string name) => + Path.Combine(System.AppContext.BaseDirectory, "Plans", name); + + private static System.Collections.Generic.IEnumerable Sessions(MainWindow window) => + window.FindControl("MainTabControl")!.Items + .OfType() + .Select(tab => tab.Content) + .OfType(); + + private static Button CompareButton(QuerySessionControl session) => + session.FindControl