From dd7c8cf871180665d47dcddb1ece812bc933f0a7 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:52:51 -0400 Subject: [PATCH] Read the IF-condition query plan and MULTIPLE PLAN statement hashes (#4468) The plan parser dropped a StmtCond IF-condition's own QueryPlan (parsed as a bare statement element with no plan of its own, per the old blanket recursion into Condition's children) and the QueryHash/QueryPlanHash of MULTIPLE PLAN statements (read only when a QueryPlan child exists). Condition/QueryPlan is now parsed with the existing cursor-path helper so its statement attributes and operator tree come from the StmtCond element; Condition/UDF sub-plans are walked too. ParseStmtAttributes now runs before the no-QueryPlan early return, so a plan-less statement still gets its hashes and other statement-level attributes. --- .../ShowPlanParserCondAndMultiplePlanTests.cs | 127 ++++++++++++++++++ .../ShowPlanParser.cs | 39 +++++- 2 files changed, 161 insertions(+), 5 deletions(-) create mode 100644 Darling/Darling.Tests/ShowPlanParserCondAndMultiplePlanTests.cs diff --git a/Darling/Darling.Tests/ShowPlanParserCondAndMultiplePlanTests.cs b/Darling/Darling.Tests/ShowPlanParserCondAndMultiplePlanTests.cs new file mode 100644 index 0000000000..bc79b7847c --- /dev/null +++ b/Darling/Darling.Tests/ShowPlanParserCondAndMultiplePlanTests.cs @@ -0,0 +1,127 @@ +/* + * 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; + +/// +/// #4468 — dropped two kinds of statement that carry their own plan or +/// hashes: a StmtCond (IF EXISTS (...)) whose condition's own QueryPlan lives under +/// Condition, and a StmtSimple with StatementType="MULTIPLE PLAN" that carries +/// QueryHash/QueryPlanHash but no QueryPlan child. +/// +/// The Condition element (StmtCondType/Condition per the showplan XSD) holds the condition's own +/// QueryPlan (0 or 1) plus optional UDF sub-plans — never a nested Stmt* element. Before the +/// fix, the old code fed each of Condition's children into the same recursive statement parser used for +/// Then/Else, so the QueryPlan element itself was handed to ParseStatement, which has +/// no QueryPlan child of its own and fell into the no-plan placeholder path: empty StatementType, +/// null hashes, a bare STATEMENT root, and the missing index and its warning silently dropped. +/// +/// Separately, ParseStatement read QueryHash/QueryPlanHash (and the rest of +/// ParseStmtAttributes) only after confirming a QueryPlan child existed, so a plan-less +/// MULTIPLE PLAN statement lost hashes it actually carries in the XML. +/// +/// The repro XML is the issue's own fixture, synthetic (dbo.t, dbo.u, [db]). +/// +public sealed class ShowPlanParserCondAndMultiplePlanTests +{ + private const string ReproXml = """ + + + + + + + + + + + """; + + private static ParsedPlan ParseAndAnalyze() + { + var plan = ShowPlanParser.Parse(ReproXml); + PlanAnalyzer.Analyze(plan); + return plan; + } + + [Fact] + public void StatementCount_IsThree_OneConditionOneThenOneMultiplePlan() + { + // StmtCond's own condition plan (1) + the Then branch's RETURN (1) + the sibling + // MULTIPLE PLAN statement (1) = 3. The condition never nests a Stmt* per the XSD, so + // it contributes exactly one statement, not zero (dropped) and not two (double-counted). + var plan = ParseAndAnalyze(); + var statements = plan.Batches.SelectMany(b => b.Statements).ToList(); + Assert.Equal(3, statements.Count); + } + + [Fact] + public void CondWithQuery_KeepsItsOwnHashesAndRealOperatorRoot() + { + var plan = ParseAndAnalyze(); + var stmt = plan.Batches.SelectMany(b => b.Statements) + .Single(s => s.StatementType == "COND WITH QUERY"); + + Assert.Equal("0x1111111111111111", stmt.QueryHash); + Assert.Equal("0x2222222222222222", stmt.QueryPlanHash); + + // The synthetic statement-type wrapper node holds the real Table Scan as its child, + // not a bare STATEMENT placeholder — the operator tree survived. + Assert.NotNull(stmt.RootNode); + var operatorChild = Assert.Single(stmt.RootNode!.Children); + Assert.Equal("Table Scan", operatorChild.PhysicalOp); + } + + [Fact] + public void CondWithQuery_KeepsItsMissingIndexSuggestion() + { + var plan = ParseAndAnalyze(); + var stmt = plan.Batches.SelectMany(b => b.Statements) + .Single(s => s.StatementType == "COND WITH QUERY"); + + var mi = Assert.Single(stmt.MissingIndexes); + Assert.Equal("dbo", mi.Schema); + Assert.Equal("t", mi.Table); + Assert.Equal(90.5, mi.Impact); + Assert.Equal("id", Assert.Single(mi.EqualityColumns)); + + // Surfaces through the batch-wide rollup the drill-down collectors and the MCP + // formatter both read (AllMissingIndexes / PlanAdvisoryAggregator) — not just parsed + // onto the statement and never surfaced anywhere. + Assert.Single(plan.AllMissingIndexes); + } + + [Fact] + public void ThenBranch_StillHasItsReturnStatement() + { + var plan = ParseAndAnalyze(); + var stmt = plan.Batches.SelectMany(b => b.Statements) + .Single(s => s.StatementType == "RETURN NONE"); + + Assert.Equal("RETURN", stmt.StatementText); + } + + [Fact] + public void MultiplePlan_KeepsItsHashesAndPlaceholderRoot() + { + var plan = ParseAndAnalyze(); + var stmt = plan.Batches.SelectMany(b => b.Statements) + .Single(s => s.StatementType == "MULTIPLE PLAN"); + + Assert.Equal("0x3333333333333333", stmt.QueryHash); + Assert.Equal("0x4444444444444444", stmt.QueryPlanHash); + + // No QueryPlan child in the XML, so the placeholder root is kept (no operator tree to show). + Assert.NotNull(stmt.RootNode); + Assert.Equal("MULTIPLE PLAN", stmt.RootNode!.PhysicalOp); + } +} diff --git a/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs b/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs index 26f2cf3b02..56ce8c164c 100644 --- a/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs +++ b/PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs @@ -79,12 +79,36 @@ private static List ParseStatementAndChildren(XElement stmtEl) if (localName == "StmtCond") { - // IF/ELSE blocks — recurse into Condition, Then, Else + // IF/ELSE blocks — recurse into Condition, Then, Else. + // XSD (StmtCondType/Condition): Condition holds the condition's OWN QueryPlan + // (0 or 1) plus optional UDF sub-plans — never a nested Stmt* element. That + // QueryPlan's statement-level facts (StatementType "COND WITH QUERY", QueryHash, + // QueryPlanHash, missing indexes, the root operator) live on the StmtCond element + // itself, so they have to be read from stmtEl, not from the QueryPlan element. var condEl = stmtEl.Element(Ns + "Condition"); if (condEl != null) { - foreach (var child in condEl.Elements()) - results.AddRange(ParseStatementAndChildren(child)); + var condQueryPlanEl = condEl.Element(Ns + "QueryPlan"); + if (condQueryPlanEl != null) + { + var condRelOpEl = condQueryPlanEl.Element(Ns + "RelOp"); + var condStmt = condRelOpEl != null + ? ParseQueryPlanAsStatement(stmtEl, condQueryPlanEl, condRelOpEl) + : ParseStatement(stmtEl); + if (condStmt != null) + results.Add(condStmt); + } + + // XSD gap: UDF sub-plans on Condition (StmtCondType/Condition/UDF) + foreach (var udfEl in condEl.Elements(Ns + "UDF")) + { + var udfStmts = udfEl.Element(Ns + "Statements"); + if (udfStmts != null) + { + foreach (var child in udfStmts.Elements()) + results.AddRange(ParseStatementAndChildren(child)); + } + } } var thenStmts = stmtEl.Element(Ns + "Then")?.Element(Ns + "Statements"); @@ -206,8 +230,13 @@ private static List ParseStatementAndChildren(XElement stmtEl) if (queryPlanEl == null) { - // Statements with no QueryPlan (e.g., DECLARE/ASSIGN) still get a synthetic - // root node so they appear in the statement tab list. + // Statements with no QueryPlan (e.g., DECLARE/ASSIGN, or a MULTIPLE PLAN statement + // whose plan was never captured) still get a synthetic root node so they appear in + // the statement tab list. ParseStmtAttributes reads only stmtEl attributes (QueryHash, + // QueryPlanHash, StatementId, etc.) — none of them depend on a QueryPlan child — so it + // runs here too, otherwise a plan-less statement loses hashes it actually carries. + ParseStmtAttributes(stmt, stmtEl); + var stmtType = stmt.StatementType.Length > 0 ? stmt.StatementType.ToUpperInvariant() : "STATEMENT";