Skip to content

Add rule 38 for standard edition limitation on batch mode when dop=2#275

Merged
erikdarlingdata merged 2 commits into
erikdarlingdata:devfrom
rferraton:rules/alert_standard_edition_dop_limit_on_batch_mode
Apr 25, 2026
Merged

Add rule 38 for standard edition limitation on batch mode when dop=2#275
erikdarlingdata merged 2 commits into
erikdarlingdata:devfrom
rferraton:rules/alert_standard_edition_dop_limit_on_batch_mode

Conversation

@rferraton
Copy link
Copy Markdown
Contributor

What does this PR do?

Add Rule 38: Standard Edition DOP 2 Limitation

  1. Analyze(ParsedPlan, AnalyzerConfig?, ServerMetadata?) method — Added optional ServerMetadata? serverMetadata parameter, passed through to AnalyzeStatement(PlanStatement, AnalyzerConfig, ServerMetadata?)
  2. Rule 38 logic in AnalyzeStatement(PlanStatement, AnalyzerConfig, ServerMetadata?) — Two scenarios:
    • With server context (repro/query session): When Edition="Standard", batch mode nodes exist, DOP=2, and MAXDOP>2 → emits a Warning confirming the Standard Edition limitation and noting that Developer/Enterprise Edition would allow higher DOP
    • Without server context (.sqlplan file): When batch mode nodes exist and DOP=2 → emits an Info suggesting the limitation may apply
  3. HasBatchModeNode(PlanNode) helper — Recursively walks the plan tree checking ActualExecutionMode or ExecutionMode for "Batch"
  4. RuleWarningTypes dictionary — Added [38] = "Standard Edition DOP Limitation" for severity override support

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

image image

Describe the testing you've done. Include:

  • Plan files tested (estimated, actual, Query Store, etc.) : Actual disconnected
  • Platforms tested (Windows, macOS, Linux) : Windows Only

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

@erikdarlingdata
Copy link
Copy Markdown
Owner

Code Review — Rule 38: Standard Edition DOP 2 limitation

Overview

Adds a new analyzer rule that flags plans with DOP=2 + batch mode where the SQL Server Standard Edition cap is the likely cause. Threads ServerMetadata? into PlanAnalyzer.Analyze and AnalyzeStatement so the rule can branch on Edition + MAXDOP. Two severities: Warning when server context confirms Standard + MAXDOP>2, Info when no server context is available. The core logic and rule fit cleanly with surrounding rules.

Issues

1. Other call sites not updated (most important)

PlanAnalyzer.Analyze is called from at least these spots, none of which now pass serverMetadata:

  • src/PlanViewer.Cli/Commands/AnalyzeCommand.cs:225, 419
  • src/PlanViewer.Cli/Commands/QueryStoreCommand.cs:289
  • src/PlanViewer.App/Mcp/McpQueryStoreTools.cs:126
  • src/PlanViewer.Web/Pages/Index.razor:1811

The optional parameter means it compiles, but the CLI Query Store path and the MCP path do connect to a live server and could plumb ServerMetadata through. As written, those code paths will only ever emit the Info-severity variant — even when running against a real Standard Edition box. At minimum the CLI Query Store/Analyze commands should be wired up; otherwise this rule is desktop-only in practice.

2. No tests

tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs has unit tests for prior rules but nothing for Rule 38. Worth adding three cases:

  • Standard + DOP=2 + batch mode + MAXDOP>2 → Warning
  • Standard + DOP=2 + batch mode + MAXDOP=2 → no warning (limitation isn't biting)
  • No server metadata + DOP=2 + batch mode → Info

PlanTestHelper would need a small overload that accepts ServerMetadata.

3. RuleWarningTypes dictionary skips 34–37 (preexisting, exacerbated)

The dictionary at PlanAnalyzer.cs:102 jumps 33 → 38. But rules 34, 35, 36, 37 exist (PlanAnalyzer.cs:1017, 1356, 440, 456) and aren't registered, so severity overrides via cfg.GetSeverityOverride silently no-op for them. Not introduced here, but adding 38 while leaving the 34–37 gap makes the inconsistency more confusing. Either add the missing entries or leave a comment noting why they're omitted.

4. Edge case: server metadata present but Edition is null/empty

if (serverMetadata != null && !string.IsNullOrEmpty(serverMetadata.Edition)
    && serverMetadata.Edition.Contains("Standard", ...))
{ /* Warning */ }
else if (serverMetadata == null)
{ /* Info */ }

If serverMetadata is non-null but Edition is null/empty (collection failure, partial metadata), neither branch fires. The intent of the Info branch — "we don't know the edition, suspect the limitation" — also applies to this case. Cleaner as:

var editionKnown = !string.IsNullOrEmpty(serverMetadata?.Edition);
if (editionKnown && serverMetadata!.Edition!.Contains("Standard", ...) && serverMetadata.MaxDop > 2) { /* Warning */ }
else if (!editionKnown) { /* Info */ }

5. Minor: HasBatchModeNode walks the tree twice

Rule 38 calls HasBatchModeNode(stmt.RootNode) and AnalyzeNodeTree already walks the same tree right after. Not a real perf concern (linear, returns on first hit), but if you wanted to be tidy, a per-statement "has batch mode" flag could be set during the existing tree walk. Optional.

Smaller notes

  • Edition string match: real values are "Standard Edition (64-bit)" etc., so Contains("Standard", OrdinalIgnoreCase) works and won't collide with Express/Web/Developer/Enterprise. Fine.
  • DegreeOfParallelism == 2 is a hard equality check — correct for this rule (the limitation only ever shows up as DOP=2).
  • Message wording is good; explicit mention of Developer/Enterprise as the workaround is useful.

Risk

Low. The change is additive, the new param is optional, and the worst case is the rule fires as Info instead of Warning until other call sites are wired up. No behavior change for existing rules.

Recommendation

Approve with requested changes:

  1. Wire ServerMetadata through the CLI commands (at least QueryStoreCommand) and ideally MCP/Web.
  2. Add a few unit tests for Rule 38.
  3. Fix the edge case where serverMetadata != null but Edition is empty.
  4. (Nit) Decide what to do about the 34–37 gap in RuleWarningTypes.

1. Fix Rule 38 edge case (PlanAnalyzer.cs)
• Introduced editionKnown variable: !string.IsNullOrEmpty(serverMetadata?.Edition)
• If ServerMetadata is non-null but Edition is null/empty (collection failure), the Info branch now fires instead of silently skipping
2. Wire RunLiveAsync(FileInfo?, string, string?, string?, DirectoryInfo?, bool, string?, bool, int, string, bool, bool, ICredentialService?, string?, string?, AnalyzerConfig) (AnalyzeCommand.cs)
• Fetches ServerMetadata via FetchServerMetadataAsync(string, bool, CancellationToken) after connection is established
• Passes it to PlanAnalyzer.Analyze(plan, analyzerConfig, serverMetadata)
• Non-fatal try/catch so analysis continues if metadata collection fails
3. Wire RunAsync(FileInfo?, bool, string, bool, bool, AnalyzerConfig) (QueryStoreCommand.cs)
• Same pattern: fetches metadata once before the plan analysis loop
• Reuses it for all plans in the batch (same server)
4. Wire McpQueryStoreTools (McpQueryStoreTools.cs)
• Fetches metadata before the LINQ Select that parses plans
• Passes via named parameter serverMetadata: serverMetadata
5. PlanTestHelper overload (PlanTestHelper.cs)
• Added LoadAndAnalyze(string planFileName, ServerMetadata? serverMetadata) overload
• Original overload delegates to the new one with null
6. Unit tests (PlanAnalyzerTests.cs) — 4 tests:
• Standard + DOP=2 + batch + MAXDOP>2 → Warning ✅
• Standard + DOP=2 + batch + MAXDOP=2 → no warning ✅
• No metadata + DOP=2 + batch → Info ✅
• Metadata with null Edition + DOP=2 + batch → Info ✅ (edge case fix)
@erikdarlingdata erikdarlingdata merged commit 6f9e2d6 into erikdarlingdata:dev Apr 25, 2026
2 checks passed
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.

2 participants