Skip to content

Plan analysis: dynamic-cursor and missing-LOCAL cursor rules (#4529) - #4549

Merged
erikdarlingdata merged 1 commit into
devfrom
plan-sync/4529-cursor
Sep 28, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
plan-sync/4529-cursor

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4529
Part of #4511

Why

PerformanceMonitor's plan analyzer parses CursorActualType and CursorName from a cursor plan but never used them: nothing warned about dynamic cursors, nothing flagged a DECLARE CURSOR missing LOCAL, and the "Scan With Predicate" rule always ended with "Check that you have appropriate indexes" even when a dynamic cursor was the actual reason no index got used.

What changes

Ports three cursor-aware rules from PerformanceStudio's PlanAnalyzer into PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs (PM keeps one file; PS has since split it into partials, but the rule bodies are unchanged):

  • Rule 36 (Dynamic Cursor): when a statement's CursorActualType is Dynamic, add a Dynamic Cursor warning naming the cursor and recommending FAST_FORWARD / STATIC / KEYSET.
  • Rule 37 (Cursor Missing LOCAL): when a DECLARE ... CURSOR ... FOR has no LOCAL between CURSOR and FOR, add a Cursor Missing LOCAL warning. The regex is ported from the corrected pattern (erikdarlingdata/PerformanceStudio@e8e5a21), not the original commit that introduced it (5019883): the original pattern looked for LOCAL before the CURSOR keyword, a position T-SQL never allows it in, so it fired on every cursor declaration, including ones already marked LOCAL. The fixed pattern captures the qualifier list between CURSOR and the introducing FOR instead. This rule scans the masked statement text (MaskCommentsAndLiterals, Rules that read the query text count hints and keywords inside comments and string literals #4524) so a DECLARE sitting inside a -- comment doesn't count.
  • Rule 11 augmentation (Scan With Predicate): when the scan's statement is running inside a dynamic cursor, the message now names the cursor as the likely cause instead of suggesting more indexes.

PM has no AnalyzerConfig, so the cfg.IsRuleDisabled(N) guards from PS were left out, matching how PM already ports other rules.

Test plan

New test class Darling/Darling.Tests/PlanSync4529Tests.cs (7 tests): rule 36 warns on Dynamic, not on Static; rule 37 warns on a bare DECLARE ... CURSOR FOR, not when LOCAL is present, and not when the DECLARE is inside a -- comment; rule 11 on a dynamic-cursor scan names the cursor and drops the index-check line, while a static-cursor scan keeps it.

Ran the plan-sync test command (Darling.Tests, in-process on macOS, Microsoft.WindowsDesktop.App stripped from the runtimeconfig):
Total: 238, Errors: 0, Failed: 0, Skipped: 2, Not Run: 0

Runtime RED on dev (pre-fix), before the product change:

Darling.Tests.PlanSync4529Tests.Rule36_DynamicCursor_Warns [FAIL]
  Assert.Contains() Failure: Filter not matched in collection
Darling.Tests.PlanSync4529Tests.Rule11_ScanWithPredicateOnDynamicCursor_NamesTheCursor [FAIL]
  Assert.Contains() Failure: Sub-string not found
Darling.Tests.PlanSync4529Tests.Rule37_CursorDeclarationWithoutLocal_Warns [FAIL]
  Assert.Contains() Failure: Filter not matched in collection
Total: 7, Errors: 0, Failed: 3, Skipped: 0, Not Run: 0

(Test file compiles unchanged against dev — it only reads pre-existing CursorActualType/CursorName members — so this is a runtime RED, not a compile RED.)

Mutations (temporary, reverted after recording RED):

  1. Rule 37's masked-text lookup replaced with the raw statement text → Rule37_CursorDeclarationInsideCommentOnly_DoesNotWarn went RED (Assert.DoesNotContain() Failure: Filter matched in collection), confirming the comment-in-text case depends on MaskCommentsAndLiterals. Reverted, rebuilt, back to green.
  2. Rule 36's CursorActualType == "Dynamic" check replaced with if (true) → Rule36_StaticCursor_DoesNotWarn went RED (Assert.DoesNotContain() Failure: Filter matched in collection), confirming the type check gates the warning. Reverted, rebuilt, back to green.

Lite.Tests build (Release, -p:EnableWindowsTargeting=true): 0 errors. Nothing in Lite/Analysis reads WarningType/rule numbers as a fixed enumerated list, so no Lite pin needed updating; grepped both test trees for WarningType/RuleNumber and found no census that enumerates rule numbers or warning-type strings to extend.

The live plan-tools class (DarlingMcpPlanToolsLivePostgresTests) skips on this rig (no Postgres fixture); CI runs it.

One deliberate difference from PerformanceStudio: rule 37 matches the statement text after MaskCommentsAndLiterals (#4524), so a DECLARE … CURSOR inside a comment or a string does not fire it. PerformanceStudio's current rule 37 still matches the raw text.

CHANGELOG

SECTION: Added
ENTRY: - Plan analysis warns on dynamic cursors and on cursors declared without LOCAL, and "Scan With Predicate" names a dynamic cursor when it is the likely cause ([#4549]) - A dynamic cursor sees data changes between fetches, which blocks many index uses. A cursor declared without LOCAL defaults to global scope. A declaration inside a comment or a string does not count.
REF: [#4549]: #4549

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 02:57
@erikdarlingdata
erikdarlingdata merged commit 7be8537 into dev Sep 28, 2026
16 of 18 checks passed
@erikdarlingdata
erikdarlingdata deleted the plan-sync/4529-cursor branch September 28, 2026 02:58
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