From 9488bf0e74597895122126b90854c97a68a87f40 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 23 Sep 2026 23:34:17 -0400 Subject: [PATCH] Leave the sp_executesql declaration list out of substituted text A plan from the plan cache or Query Store records an sp_executesql statement with its declaration list in front: "(@p1 int, @p2 int)SELECT". Substitution read those names as references, so the advice, exports, MCP tools, "Copy Query Text (with values)" and planview's text output showed "(10 int, 20 int)SELECT ...", which neither matches the plan nor runs. Only the statement after the list gets values now, and when it gets any, the list is dropped. With no values the text stays as the plan recorded it. A list that never closes (the plan cut the text off at 4,000 characters inside it) is left alone. parameterized_statement_text still carries the original. ReproScriptBuilder had its own copy of the list parser; both now use ParameterSubstitution.DeclarationListEnd, which the web project can see. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_017ZVrq8tpA2DBPEFEqahFK6 --- .../Services/ParameterSubstitution.cs | 73 +++++++++++++++---- .../Services/ReproScriptBuilder.cs | 34 +-------- .../ComparisonBaseline.txt | 8 +- .../ParameterSubstitutionTests.cs | 66 +++++++++++++++++ .../ReproScriptBuilderTests.cs | 46 ++++++++++++ 5 files changed, 179 insertions(+), 48 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/ReproScriptBuilderTests.cs diff --git a/src/PlanViewer.Core/Services/ParameterSubstitution.cs b/src/PlanViewer.Core/Services/ParameterSubstitution.cs index 3ab9c52..0e3a2e9 100644 --- a/src/PlanViewer.Core/Services/ParameterSubstitution.cs +++ b/src/PlanViewer.Core/Services/ParameterSubstitution.cs @@ -47,46 +47,60 @@ public static ParameterSubstitutionResult Apply( if (values.Count == 0) return new ParameterSubstitutionResult(statementText, 0); + /* A plan from the plan cache or Query Store keeps an sp_executesql statement's declaration + list in front of it: "(@p1 int, @p2 int)SELECT …". The names in that list declare the + parameters, they do not read them. Substituted, they became "(10 int, 20 int)SELECT …", + which is neither the plan's text nor runnable. So only the statement after the list gets + values, and when it gets any, the list is dropped: this text is meant to run, and a + declaration list is not T-SQL on its own. Nothing substituted leaves the text as it was. + A list that never closes is text the plan cut off at 4,000 characters inside the list, + so there is no statement to put values into. */ + var bodyStart = DeclarationListEnd(statementText); + if (bodyStart < 0) + return new ParameterSubstitutionResult(statementText, 0); + + var text = bodyStart == 0 ? statementText : statementText[bodyStart..].TrimStart(); + /* One fact about the whole statement, settled up front: inside an EXEC statement, every token sitting to the left of an "=" is an assignment target — the return-status variable or a named argument's name — because EXEC grammar has no other use for "=" at all. A per-token back-scan cannot see this for the FIRST named argument (what precedes it is the procedure name, not a keyword), which is how "EXEC dbo.p @debug = @debug" got its left-hand side substituted into "EXEC dbo.p 1 = @debug". */ - var assignsThroughEquals = StatementLeadsWithExec(statementText); + var assignsThroughEquals = StatementLeadsWithExec(text); - var sb = new StringBuilder(statementText.Length); + var sb = new StringBuilder(text.Length); var substitutions = 0; var i = 0; - while (i < statementText.Length) + while (i < text.Length) { - var c = statementText[i]; + var c = text[i]; /* Regions where an @name is text, not a parameter. A string literal is the case that matters in practice — LIKE 'kexin%' sits right next to the parameters in the #466 repro — but a delimited identifier can hold anything, and a comment is not code. */ if (c == '\'' || c == '"') { - i = CopyDelimited(statementText, i, c, c, sb); + i = CopyDelimited(text, i, c, c, sb); continue; } if (c == '[') { - i = CopyDelimited(statementText, i, '[', ']', sb); + i = CopyDelimited(text, i, '[', ']', sb); continue; } - if (c == '-' && i + 1 < statementText.Length && statementText[i + 1] == '-') + if (c == '-' && i + 1 < text.Length && text[i + 1] == '-') { - i = CopyLineComment(statementText, i, sb); + i = CopyLineComment(text, i, sb); continue; } - if (c == '/' && i + 1 < statementText.Length && statementText[i + 1] == '*') + if (c == '/' && i + 1 < text.Length && text[i + 1] == '*') { - i = CopyBlockComment(statementText, i, sb); + i = CopyBlockComment(text, i, sb); continue; } @@ -94,15 +108,15 @@ matters in practice — LIKE 'kexin%' sits right next to the parameters in the # tail of an identifier such as "t@0" is part of that identifier. The scan below claims the longest run of identifier characters, which handles the first; the preceding character is checked here, which handles the second. */ - if (c == '@' && !IsIdentifierPart(i > 0 ? statementText[i - 1] : '\0')) + if (c == '@' && !IsIdentifierPart(i > 0 ? text[i - 1] : '\0')) { var end = i + 1; - while (end < statementText.Length && IsIdentifierPart(statementText[end])) + while (end < text.Length && IsIdentifierPart(text[end])) end++; - var token = statementText[i..end]; + var token = text[i..end]; if (values.TryGetValue(token, out var value) - && !IsAssignmentTarget(statementText, i, end, assignsThroughEquals)) + && !IsAssignmentTarget(text, i, end, assignsThroughEquals)) { sb.Append(value); substitutions++; @@ -125,6 +139,37 @@ tail of an identifier such as "t@0" is part of that identifier. The scan below c : new ParameterSubstitutionResult(sb.ToString(), substitutions); } + /// + /// Where the statement starts in text that opens with an sp_executesql declaration list, + /// such as (@p1 int, @p2 decimal(18,2))SELECT …. Returns 0 when the text has no list, + /// and -1 when the list never closes: a plan cuts statement text off at 4,000 characters, and + /// the declarations for a long IN list can fill all of them. The list ends at the parenthesis + /// that closes its first one, so the parentheses of a type are counted, not taken for the end. + /// A statement cannot begin with (@, so that opening always means a list. + /// ReproScriptBuilder strips the list with this too. (The web project compiles this + /// file without it, so the name is not a cref.) + /// + internal static int DeclarationListEnd(string text) + { + var i = 0; + while (i < text.Length && char.IsWhiteSpace(text[i])) + i++; + + if (i + 1 >= text.Length || text[i] != '(' || text[i + 1] != '@') + return 0; + + var depth = 0; + for (; i < text.Length; i++) + { + if (text[i] == '(') + depth++; + else if (text[i] == ')' && --depth == 0) + return i + 1; + } + + return -1; // the list never closes: the text was cut off inside it + } + /// /// True when the parameter token spanning to is /// being assigned TO rather than read from, in which case its value must not be written over it. diff --git a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs index c19569c..a7f8a59 100644 --- a/src/PlanViewer.Core/Services/ReproScriptBuilder.cs +++ b/src/PlanViewer.Core/Services/ReproScriptBuilder.cs @@ -365,39 +365,13 @@ private static List ExtractSetOptionsFromPlan(string planXml) /// /// Strips the parameter declaration prefix from query text captured via sp_executesql. /// Query text like "(@p1 int, @p2 nvarchar(50))SELECT ..." becomes "SELECT ...". - /// Uses same approach as sp_QueryReproBuilder: find the closing ) followed by non-comma. + /// The list is found by the same parser that keeps parameter substitution out of it. /// private static string StripParameterPrefix(string queryText) { - if (!queryText.StartsWith("(@", StringComparison.Ordinal)) - { - return queryText; - } - - /* Find the closing parenthesis that ends the parameter list. - Look for ) followed by a character that's not a comma (which would indicate - we're still inside nested parentheses in a type like decimal(18,2)). */ - int depth = 0; - for (int i = 0; i < queryText.Length; i++) - { - char c = queryText[i]; - if (c == '(') - { - depth++; - } - else if (c == ')') - { - depth--; - if (depth == 0) - { - /* Found the closing paren — return everything after it, trimmed */ - return queryText[(i + 1)..].TrimStart(); - } - } - } - - /* Couldn't find balanced parens — return original */ - return queryText; + /* No list (0), or a list that never closes (-1): the text stays as it is, as before. */ + var bodyStart = ParameterSubstitution.DeclarationListEnd(queryText); + return bodyStart <= 0 ? queryText : queryText[bodyStart..].TrimStart(); } /// diff --git a/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt b/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt index 96ce828..e95b56e 100644 --- a/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt +++ b/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt @@ -312,7 +312,7 @@ Plan A: in_list_dynamic_seek_plan.sqlplan Plan B: in_list_dynamic_seek_plan.sqlplan --- Statement 1 --- -(10 int, 20 int)SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20) +SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20) Estimated cost: 0.0033 -> 0.0033 (0.0% costlier) Estimated rows: 2 -> 2 (0.0% more) @@ -697,7 +697,7 @@ Plan A: non_sargable_compound_predicate_plan.sqlplan Plan B: non_sargable_compound_predicate_plan.sqlplan --- Statement 1 --- -(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) +SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) Estimated cost: 0.0033 -> 0.0033 (0.0% costlier) Estimated rows: 1 -> 1 (0.0% more) @@ -1500,7 +1500,7 @@ Plan A: in_list_dynamic_seek_plan.sqlplan Plan B: isnull_plan.sqlplan --- Statement 1 --- -(10 int, 20 int)SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20) +SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20) Estimated cost: 0.0033 -> 3,119.42 (9,999% costlier) Estimated rows: 2 -> 1 (50.0% fewer) @@ -1904,7 +1904,7 @@ Plan A: non_sargable_compound_predicate_plan.sqlplan Plan B: non_sargable_function_plan.sqlplan --- Statement 1 --- -(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) +SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) Estimated cost: 0.0033 -> 3,097.52 (9,999% costlier) Estimated rows: 1 -> 1 (0.0% more) diff --git a/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs b/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs index d6440d6..7ab6ee8 100644 --- a/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs +++ b/tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs @@ -390,4 +390,70 @@ parameter name survives anywhere in the text. */ Assert.Contains("like 'kexin%'", result.Text); Assert.DoesNotContain("@", result.Text); } + + [Fact] + public void DeclarationList_IsLeftOutAndNotSubstituted() + { + /* A plan from the plan cache or Query Store keeps an sp_executesql statement's declaration + list in front of it. Substituted like the rest, "(@p1 int, @p2 int)" became + "(10 int, 20 int)", which is neither the plan's text nor runnable. */ + var result = ParameterSubstitution.Apply( + "(@p1 int, @p2 int)SELECT t.Id FROM dbo.T AS t WHERE t.A IN (@p1, @p2)", + new List { Param("@p1", "(10)"), Param("@p2", "(20)") }); + + Assert.Equal("SELECT t.Id FROM dbo.T AS t WHERE t.A IN (10, 20)", result.Text); + Assert.Equal(2, result.SubstitutionCount); + } + + [Fact] + public void DeclarationListWithParenthesizedTypes_EndsAtItsOwnClosingParenthesis() + { + /* The comma inside decimal(18,2) and the parentheses of both types are part of the list. + Stopping at the first closing parenthesis would leave ",@b nvarchar(50))" in the text. */ + var result = ParameterSubstitution.Apply( + "(@a decimal(18,2),@b nvarchar(50))SELECT * FROM t WHERE x = @a AND y = @b", + new List { Param("@a", "(1.50)"), Param("@b", "N'abc'") }); + + Assert.Equal("SELECT * FROM t WHERE x = 1.50 AND y = N'abc'", result.Text); + } + + [Fact] + public void DeclarationList_StaysWhenNothingIsSubstituted() + { + /* With no value to put back, the text is shown as the plan recorded it, list included. */ + const string text = "(@p1 int)SELECT 1"; + var result = ParameterSubstitution.Apply(text, new List { Param("@p1", "(10)") }); + + Assert.Equal(text, result.Text); + Assert.Equal(0, result.SubstitutionCount); + } + + [Fact] + public void DeclarationListCutOffByTruncation_IsLeftAsItIs() + { + /* A plan cuts statement text off at 4,000 characters, and the declarations for a long IN + list can fill all of them. Then the text is only declarations, with no statement after + them, and putting values into it would make "(1 int,2 int,…" again. */ + const string text = "(@p0 int,@p1 int,@p2 in"; + var result = ParameterSubstitution.Apply( + text, new List { Param("@p0", "(1)"), Param("@p1", "(2)") }); + + Assert.Equal(text, result.Text); + Assert.Equal(0, result.SubstitutionCount); + } + + [Fact] + public void AutoParameterizedPlan_LosesItsDeclarationList() + { + /* The #556 reproduction, an auto-parameterized plan: its text starts with + "(@1 tinyint,@2 int)", and the comparison report printed "(52 tinyint,2 int)SELECT". */ + var plan = PlanTestHelper.LoadAndAnalyze("non_sargable_compound_predicate_plan.sqlplan"); + var statement = PlanTestHelper.FirstStatement(plan); + Assert.StartsWith("(@1 tinyint,@2 int)", statement.StatementText); + + var result = ParameterSubstitution.Apply(statement.StatementText, statement.Parameters); + + Assert.StartsWith("SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52", result.Text); + Assert.DoesNotContain("@", result.Text); + } } diff --git a/tests/PlanViewer.Core.Tests/ReproScriptBuilderTests.cs b/tests/PlanViewer.Core.Tests/ReproScriptBuilderTests.cs new file mode 100644 index 0000000..9efbb2e --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ReproScriptBuilderTests.cs @@ -0,0 +1,46 @@ +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// The repro script declares the parameters itself, so the declaration list that a cached +/// sp_executesql statement starts with must not reach the script's query text. The list is found +/// by the parser that parameter substitution uses (ParameterSubstitution.DeclarationListEnd). +/// +public class ReproScriptBuilderTests +{ + private const string Plan = """ + + + + + + + + + + + + """; + + [Fact] + public void BuildReproScript_DeclarationList_IsLeftOutOfTheQueryText() + { + var sql = ReproScriptBuilder.BuildReproScript( + "(@id decimal(18,2))SELECT * FROM dbo.T WHERE Id = @id", "db", Plan, null); + + Assert.Contains("SELECT * FROM dbo.T WHERE Id = @id", sql); + Assert.DoesNotContain("(@id decimal(18,2))SELECT", sql); + Assert.Contains("@id = 42.50", sql); + } + + [Fact] + public void BuildReproScript_DeclarationListCutOffByTruncation_IsKeptAsItIs() + { + /* The parser reports a list that never closes as -1. The text stays as it was, as it did + before the parser was shared, and the -1 must never reach a slice. */ + var sql = ReproScriptBuilder.BuildReproScript("(@id decimal(18,2),@x nvarch", "db", Plan, null); + + Assert.Contains("(@id decimal(18,2),@x nvarch", sql); + } +}