Skip to content

Descend into UDF and procedure bodies inside cursor plans - #493

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/cursor-sub-plans
Sep 3, 2026
Merged

Descend into UDF and procedure bodies inside cursor plans#493
erikdarlingdata merged 1 commit into
devfrom
fix/cursor-sub-plans

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #491.

#456 taught the parser to descend into StoredProc/UDF sub-plans, but that descent lived in ParseStatement — a StmtCursor's operation statements are built through ParseQueryPlanAsStatement and never pass through it, so a function called by a cursor's query carried its whole body in the XML and the parser dropped every statement of it.

The two element-reads are extracted into one shared ParseSubPlans(stmt, containerEl, depth, ct) — byte-identical logic — called from ParseStatement (unchanged semantics) and now from the cursor branch per Operation element, where the real schema places the UDF sub-plan beside the QueryPlan. PlanStatements.EnumerateAll already walks UdfPlans/StoredProcPlan on every statement, and since #486 every consumer reads that traversal, so cursor bodies surface everywhere (analyzer, scorer, grid, web) with zero traversal changes — pinned end-to-end by a test running Analyze + Score + ResultMapper.Map and asserting the counted statements.

Depth passes through unreset (#484 preserved): an alternating cursor/procedure depth bomb throws the catchable depth error, and the regression mode was verified honestly — with the descent temporarily reset to depth: 0, the bomb test fails on its assert without killing the host, then the patch was reverted.

Fixtures are generated in-test with justifying comments (the depth-limit tests' precedent): no committed cursor fixture exists at all, and udf_plan.sqlplan calls a UDF without carrying a sub-plan element.

How was this tested?

Six new tests in CursorSubPlanTests and ShowPlanParserLimitsTests: attach-point, EnumerateAll order + container paths, analyzed-and-counted via ResultMapper, the Operation-level StoredProc symmetric read, the alternating depth bomb, and 50-level legitimate nesting parsing every level. Full suite at dev tip: 451 tests, 450 passed, 1 platform skip, 0 failed — run twice in the worktree and once in the main checkout.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n

#456 taught the parser to read StoredProc/UDF sub-plan bodies, but that
descent lives in ParseStatement - and a StmtCursor's operation statements
never pass through ParseStatement. They are built in the cursor branch of
ParseStatementAndChildren straight from CursorPlan > Operation > QueryPlan,
so a function called by the cursor's query carried its whole body in the
XML (the Operation element holds the UDF sub-plan beside its QueryPlan)
and the parser dropped every statement of it: not enumerated, not
analyzed, not counted. Same failure mode as #455 - well-formed, plausible,
and silently incomplete output.

Fix (#491): extract ParseStatement's UDF/StoredProc reads into a shared
ParseSubPlans helper and call it from the cursor branch on each Operation
element, attaching the bodies to that operation's statement. That is the
whole integration: PlanStatements.EnumerateAll already walks
UdfPlans/StoredProcPlan on every statement it yields, and since #486 every
consumer (analyzer, scorer, result mapper, statements grid, web viewer)
reads that traversal, so the bodies flow through analysis and counts with
no consumer changes.

The caller's depth carries into the new descent unchanged, preserving
#484: a generated bomb alternating cursor and procedure shapes past
MaxParseDepth throws the catchable depth error on the big-stack thread,
and was verified to fail cleanly (parse completes, assert reports the
miss) against a simulated depth reset at the cursor boundary. Tests use
generated XML on the depth-limit tests' precedent - a cursor wrapping a
UDF sub-plan is three nested element shapes, stated more clearly by a
minimal document than a captured fixture.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, narrowly-scoped fix — no correctness issues found.

Verified specifics:

  • ParseSubPlans extraction is a byte-identical refactor of the two existing UDF/StoredProc read blocks (confirmed against the pre-PR code); the only change is the parameter rename stmtElcontainerEl, so ParseStatement's existing behavior is unaffected.
  • The new call site correctly reads from opEl (the Operation element), which is where UDF/StoredProc actually sit next to QueryPlan in the real showplan schema for cursor operations — not from the outer StmtCursor element, which would have been a no-op.
  • Depth handling is consistent with the existing ParseStatement call: both pass the caller's depth unchanged into ParseSubPlans, and the actual "one level deeper" increment happens only on the recursive ParseStatementAndChildren(childStmt, depth + 1, ...) call inside it. So #484's depth-carry guarantee genuinely extends to the cursor branch rather than just asserting it in a comment.
  • Traced PlanStatements.EnumerateAll/EnumerateAllWithContainer and confirmed no consumer changes were actually needed — analyzer, scorer, ResultMapper, and the statements grid all walk that shared traversal already, so the fix's blast radius really is contained to the parser.
  • New depth-limit tests correctly build alternating cursor/procedure nesting (verified the level-parity math for both the BombDepth and 50-level legitimate-nesting cases) and use a dedicated 8MB-stack thread for the bomb case, matching the existing NestedProcedurePlan bomb test's approach.

No untrusted-input/SQL-generation concerns (parser-only change, no T-SQL emitted), no version-bump or PlanViewer.Web linked-file implications (only ShowPlanParser.cs and two test files touched).

One pre-existing (not introduced by this PR) limitation worth noting for future work: a cursor Operation with no QueryPlan/RelOp still skips sub-plan attachment entirely via the early continues at lines ~200-204, same as before this fix — so a UDF called from such an operation would still be dropped. Out of scope for #491 as scoped, but worth a follow-up issue if that shape turns out to occur in practice.

@erikdarlingdata
erikdarlingdata merged commit 6ae2634 into dev Sep 3, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/cursor-sub-plans branch September 3, 2026 14:23
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