Skip to content

The plan parser reads IF-condition query plans and MULTIPLE PLAN statement hashes (#4468) - #4470

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/4468-showplan-cond-and-multiple-plan
Sep 27, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/4468-showplan-cond-and-multiple-plan

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4468.

Why

ShowPlanParser.Parse dropped two kinds of statement that carry their own plan or hashes:

  1. StmtCond with StatementType="COND WITH QUERY" (an IF EXISTS (SELECT …) condition): the condition's
    own QueryPlan, under <Condition>, was never parsed as a statement. It came back with an empty
    StatementType, null QueryHash/QueryPlanHash, and a placeholder STATEMENT root — its real operator
    tree, its missing-index suggestions, and every warning PlanAnalyzer would raise on them were lost.
  2. StmtSimple with StatementType="MULTIPLE PLAN": a statement element that carries QueryHash and
    QueryPlanHash but no QueryPlan child came back with both hashes null.

Both hashes are what Query Store and plan-cache matching join on, so a batch with either shape under-reported
its own statements to every downstream reader: the plan-analysis MCP tools, the drill-down collectors that
aggregate missing indexes and warnings across a set of plans, and the plan viewer.

What changes

PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs:

  • ParseStatementAndChildren (StmtCond branch): per the showplan XSD (StmtCondType/Condition), Condition
    holds the condition's own QueryPlan (0 or 1) plus optional UDF sub-plans — never a nested Stmt* element.
    The old code recursed into Condition's children with the same statement parser used for Then/Else, which
    handed the bare QueryPlan element to ParseStatement (no attributes of its own to read). The fix reads the
    condition's QueryPlan directly and parses it with the existing ParseQueryPlanAsStatement helper (already
    used for the cursor path), so its StatementText, StatementType, QueryHash, QueryPlanHash, and the rest
    of ParseStmtAttributes all come from the StmtCond element, and its plan comes from Condition/QueryPlan.
    Condition/UDF sub-plans are now also walked. Then/Else recursion is unchanged.
  • ParseStatement: ParseStmtAttributes (which reads QueryHash, QueryPlanHash, StatementId, and the
    rest of the statement-level attributes — none of which depend on a QueryPlan child existing) now runs before
    the no-QueryPlan early return, not only after it. The synthetic placeholder root node is unchanged.

PerformanceStudio's parser (src/PlanViewer.Core/Services/ShowPlanParser.cs in erikdarlingdata/PerformanceStudio) has the same StmtCond
recursion shape. This PR does not change it.

Test plan

New file Darling/Darling.Tests/ShowPlanParserCondAndMultiplePlanTests.cs, five facts against the issue's own
repro XML (synthetic dbo.t, dbo.u, [db]):

  • StatementCount_IsThree_OneConditionOneThenOneMultiplePlan — the batch parses to exactly 3 statements
    (the condition's own plan, the Then branch's RETURN, and the MULTIPLE PLAN sibling).
  • CondWithQuery_KeepsItsOwnHashesAndRealOperatorRoot — QueryHash/QueryPlanHash match the StmtCond
    element's own hashes, and the statement's root wraps the real Table Scan RelOp, not a bare placeholder.
  • CondWithQuery_KeepsItsMissingIndexSuggestion — the [t]/[id] equality missing index (impact 90.5) is
    parsed onto the statement AND surfaces through the batch-wide ParsedPlan.AllMissingIndexes rollup that the
    drill-down collectors and PlanAdvisoryAggregator both read.
  • ThenBranch_StillHasItsReturnStatement — the Then branch's RETURN NONE statement is unaffected.
  • MultiplePlan_KeepsItsHashesAndPlaceholderRoot — the MULTIPLE PLAN statement's hashes are present and its
    placeholder root (no operator tree, since there's no QueryPlan) is kept.

RED on origin/dev (git worktree add --detach at 2a33914ae, test file copied in, no other change):
3 of 5 facts failed at runtime (CondWithQuery_KeepsItsOwnHashesAndRealOperatorRoot,
CondWithQuery_KeepsItsMissingIndexSuggestion, MultiplePlan_KeepsItsHashesAndPlaceholderRoot) — 2/5 passed
because they don't depend on either bug. Total: 5, Errors: 0, Failed: 3.

Mutations (each applied, run, reverted):

  • Reverting the StmtCond/Condition branch to the old blanket recursion → the two CondWithQuery_* facts
    went RED (Total: 5, Failed: 2); reverted.
  • Moving ParseStmtAttributes(stmt, stmtEl) back behind the queryPlanEl == null return in ParseStatement →
    MultiplePlan_KeepsItsHashesAndPlaceholderRoot went RED (Total: 5, Failed: 1); reverted.

Green after the fix, on this Mac (Darling.Tests.dll in-process, Microsoft.WindowsDesktop.App framework
entry removed from the runtimeconfig)
: the new class (5) plus McpPlanAnalysisEnvelopeTests (the other
Darling.Tests class touching PlanAnalysis) and DocCommentHygieneTests — Total: 88, Errors: 0, Failed: 0.

Both Darling.Tests and Lite.Tests build Release with -p:EnableWindowsTargeting=true: 0 warnings, 0 errors.
No Lite-specific parser tests exist to name (Lite.Tests has no ShowPlanParser/PlanAnalysis-named test
file); Lite.Tests.dll doesn't run in-process on this Mac (WindowsBase discovery failure) — this change carries
no Lite-only behavior, so CI's normal run is the only additional signal.

CHANGELOG entry

SECTION: Fixed
ENTRY:

…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.
@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 27, 2026 16:05
@erikdarlingdata
erikdarlingdata merged commit 6f50c73 into dev Sep 27, 2026
16 of 18 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4468-showplan-cond-and-multiple-plan branch September 27, 2026 16:05
erikdarlingdata added a commit to nmummau/PerformanceStudio that referenced this pull request Sep 29, 2026
…ata#580)

A StmtCond keeps its condition's own QueryPlan (and any UDF sub-plans)
under <Condition>. The parser fed each child of <Condition> back in as a
statement, so the condition became an empty STATEMENT placeholder and its
operator tree, hashes, missing indexes and warnings were lost. Parse the
StmtCond itself as the statement and read its plan and sub-plans from
<Condition> (new optional planContainerEl argument on ParseStatement).

ParseStatement read the statement attributes only after the no-QueryPlan
return, so a MULTIPLE PLAN statement lost the QueryHash and QueryPlanHash
it carries. Read them before that return.

Port of erikdarlingdata/PerformanceMonitor#4470. The two golden baselines
change because eager_table_spool_plan.sqlplan has a WHILE (SELECT ...)
condition that is now a real statement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant