Repository navigation
Plan analysis: dynamic-cursor and missing-LOCAL cursor rules (#4529) - #4549
Merged
Merged
Conversation
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 02:57
This was referenced Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4529
Part of #4511
Why
PerformanceMonitor's plan analyzer parses
CursorActualTypeandCursorNamefrom a cursor plan but never used them: nothing warned about dynamic cursors, nothing flagged aDECLARE CURSORmissingLOCAL, 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
PlanAnalyzerintoPerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs(PM keeps one file; PS has since split it into partials, but the rule bodies are unchanged):CursorActualTypeisDynamic, add aDynamic Cursorwarning naming the cursor and recommendingFAST_FORWARD/STATIC/KEYSET.DECLARE ... CURSOR ... FORhas noLOCALbetweenCURSORandFOR, add aCursor Missing LOCALwarning. The regex is ported from the corrected pattern (erikdarlingdata/PerformanceStudio@e8e5a21), not the original commit that introduced it (5019883): the original pattern looked forLOCALbefore theCURSORkeyword, a position T-SQL never allows it in, so it fired on every cursor declaration, including ones already markedLOCAL. The fixed pattern captures the qualifier list betweenCURSORand the introducingFORinstead. 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 aDECLAREsitting inside a--comment doesn't count.PM has no
AnalyzerConfig, so thecfg.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 onDynamic, not onStatic; rule 37 warns on a bareDECLARE ... CURSOR FOR, not whenLOCALis present, and not when theDECLAREis 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.Appstripped from the runtimeconfig):Total: 238, Errors: 0, Failed: 0, Skipped: 2, Not Run: 0Runtime RED on dev (pre-fix), before the product change:
(Test file compiles unchanged against dev — it only reads pre-existing
CursorActualType/CursorNamemembers — so this is a runtime RED, not a compile RED.)Mutations (temporary, reverted after recording RED):
Rule37_CursorDeclarationInsideCommentOnly_DoesNotWarnwent RED (Assert.DoesNotContain() Failure: Filter matched in collection), confirming the comment-in-text case depends onMaskCommentsAndLiterals. Reverted, rebuilt, back to green.CursorActualType == "Dynamic"check replaced withif (true)→Rule36_StaticCursor_DoesNotWarnwent RED (Assert.DoesNotContain() Failure: Filter matched in collection), confirming the type check gates the warning. Reverted, rebuilt, back to green.Lite.Testsbuild (Release,-p:EnableWindowsTargeting=true): 0 errors. Nothing inLite/AnalysisreadsWarningType/rule numbers as a fixed enumerated list, so no Lite pin needed updating; grepped both test trees forWarningType/RuleNumberand 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 aDECLARE … CURSORinside 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 withoutLOCALdefaults to global scope. A declaration inside a comment or a string does not count.REF: [#4549]: #4549