Skip to content

Plan analysis: adaptive join note, truncated-text rule, UDF/serial dedup, invariant percentages (#4531, #4532, #4533, #4536) - #4548

Merged
erikdarlingdata merged 3 commits into
devfrom
plan-sync/4531-4536
Sep 28, 2026
Merged

erikdarlingdata merged 3 commits into
devfrom
plan-sync/4531-4536

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4531
Fixes #4532
Fixes #4533
Fixes #4536
Part of #4511

Why

Four small findings in plan analysis carried forward from PerformanceStudio (PS):

  • An adaptive join sizes its memory grant for its hash join branch. When it runs as Nested Loops instead, most of that grant goes unused — expected, but the Excessive Memory Grant finding didn't say so, so the reader could chase a memory-grant fix that doesn't apply.
  • SQL Server caps a statement's copy of its query text at about 4,000 characters in showplan XML. Rule 3's MAXDOP message already checked for this, but nothing on the plan told the reader when the text they're looking at is short.
  • When a scalar UDF prevents parallelism, rule 3 already reports a Serial Plan finding that names the UDF as the cause. Rule 6 then added a second, separate Scalar UDF finding for the same UDF — two findings for one cause.
  • Three messages formatted a percentage with .NET's P0 format, which is culture-sensitive in the space before the % sign (100% on en-US, 100 % under InvariantCulture). That makes warning text differ by machine.

What changes

Mirrors these PerformanceStudio commits, adapted to PM's single PlanAnalyzer.cs (PS splits the analyzer into partial files) and with no AnalyzerConfig/IsRuleDisabled guard (PM has none):

  • erikdarlingdata/PerformanceStudio@4a5efa5 (adaptive-join part only): the Excessive Memory Grant finding now adds a note when an adaptive join executed as Nested Loops at runtime, via HasAdaptiveJoinChoseNestedLoop (node.IsAdaptive and node.ActualJoinType containing "Nested"). That commit's Serial Plan benefit-formula rework is a separate issue and isn't part of this change.
  • erikdarlingdata/PerformanceStudio@35249e2 (rule-39 part only): PlanStatement.IsTextTruncated (length ≥ 3,990) replaces the inline length check rule 3 already had, and a new rule 39 adds an Info finding per statement whose text hit the cap. The full-text-recovery half of that commit needs a captured-query buffer PM has no equivalent for, and isn't part of this port.
  • erikdarlingdata/PerformanceStudio@495750f plus @cc18844's Lite: MCP tool coverage gaps — 8 collectors with no tools #576 fix: rule 6 now skips its Scalar UDF finding when the statement already carries a Serial Plan finding for one of the UDF-related non-parallel reasons (TSQLUserDefinedFunctionsNotParallelizable, CLRUserDefinedFunctionRequiresDataAccess, CouldNotGenerateValidParallelPlan). Both conditions are checked together — checking only the reason would drop the UDF finding whenever rule 3 was disabled or gated out, with nothing replacing it.
  • erikdarlingdata/PerformanceStudio@582c88e: the three {pct:P0}/{skewRatio:P0} sites (the two spill "Operator time" messages and the parallel thread-skew message) now format as {pct * 100:N0}%, which is culture-independent.

No change to AnalyzerConfig, BenefitScorer, or any store rung. Consumers (PlanAdvisoryAggregator, the MCP plan tools, Lite's analysis, the plan viewer) read PlanWarning/PlanStatement through the same shapes; nothing new is exposed beyond PlanStatement.IsTextTruncated/TruncationLengthThreshold and the new "Truncated Query Text" warning type.

Test plan

New tests, one class per issue, construct a ParsedPlan/PlanStatement/PlanNode directly and run it through PlanAnalyzer.Analyze — the same pattern as the existing PlanSync4524Tests:

  • PlanSync4531Tests: an adaptive join with ActualJoinType containing "Nested" plus an excessive grant gets the note; the same shape with a Hash Match ActualJoinType, or a non-adaptive join, does not.
  • PlanSync4532Tests: IsTextTruncated at/under the 3,990 threshold; a 3,995-character statement gets the rule-39 Info finding, a 3,000-character one does not.
  • PlanSync4533Tests: a serial plan with NonParallelPlanReason = TSQLUserDefinedFunctionsNotParallelizable (and the CLR variant) suppresses rule 6; the same UDF in a plan with no non-parallel reason (so no Serial Plan finding) still gets the rule-6 finding.
  • PlanSync4536Tests: runs the parallel-skew path under CultureInfo.InvariantCulture and asserts the message contains 100% with no space.

Runtime RED on dev (each new file in a detached worktree at dev):

  • PlanSync4531Tests.AdaptiveJoinRanNestedLoops_NotesTheUnusedGrant: Assert.Contains() Failure … Not found: "adaptive join".
  • PlanSync4533Tests: both UDF-reason facts fail with Expected: False, Actual: True (rule 6 still fired).
  • PlanSync4536Tests: … "Thread 1 processed 100 % of rows …".
  • PlanSync4532Tests does not compile on dev because two facts name the new PlanStatement.IsTextTruncated member. Its two rule-39 facts use only existing members, and dev has no "Truncated Query Text" rule, so they fail there by construction; the rule-39 mutation below shows the same failure.

Mutations, each reverted: HasAdaptiveJoinChoseNestedLoop short-circuited to false (#4531); the rule-39 condition short-circuited to false (#4532); the rule-6 suppression disabled (#4533); P0 restored at the thread-skew site (#4536). Each fails its issue's test.

Run: the four new classes plus the plan-analysis classes: Total: 242, Failed: 0, Skipped: 2 (the live-Postgres plan-tool class runs in CI). Lite.Tests builds. No test enumerates warning types, so no census pin changed.

Not ported: the full-query-text recovery half of PerformanceStudio@35249e2, which needs a captured query buffer PM does not have.

CHANGELOG

SECTION: Fixed
ENTRY: - Plan analysis: adaptive-join memory grants, redundant UDF findings, and culture-dependent percentages ([#4548]) - The Excessive Memory Grant finding now notes when an adaptive join ran as Nested Loops, so the unused grant isn't mistaken for a separate problem. Rule 6's Scalar UDF finding no longer duplicates a Serial Plan finding that already names the same UDF as the reason a plan runs serially. Three warning messages that reported a percentage now format it the same way regardless of the machine's locale.
REF: [#4548]: #4548

SECTION: Added
ENTRY: - Plan analysis flags a query whose text was cut off by SQL Server's showplan limit ([#4548]) - When a statement's copy of its query text hit SQL Server's roughly 4,000-character cap when the plan was captured, the analysis now says so instead of leaving the reader to work out why re-running or formatting that text fails.
REF: [#4548]: #4548

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 03:22
@erikdarlingdata
erikdarlingdata merged commit 6b48a04 into dev Sep 28, 2026
15 of 16 checks passed
@erikdarlingdata
erikdarlingdata deleted the plan-sync/4531-4536 branch September 28, 2026 03:22
erikdarlingdata added a commit that referenced this pull request Sep 28, 2026
…00Z UTC (#4621)

Moves the CHANGELOG entries carried in merged pull-request descriptions into [Unreleased]. The cut is PRs merged after 2026-09-26T17:37:33Z and at or before 2026-09-28T17:40:00Z; the next splice starts after it.

- 76 PRs are spliced: Fixed 36, Changed 28 and Added 12, each counted once under its first section. That is 79 bullets: #4481's Fixed entry holds three, and #4548 adds a second group under Added.
- 21 PRs have no user-visible entry (None, test-only, CI-only, or deferred to a parent).
- The [#4509] link definition, which #4538 also carries, is defined once.
- The IMPORTANT upgrade notes from #4489, #4495, #4501, #4506 and #4541 are held for the release cut and are not in this change.
- Only CHANGELOG.md changes.
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