Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL Server: Possible off-by one in batch splitting logic #13639

Closed
divega opened this issue Oct 16, 2018 · 3 comments
Closed

SQL Server: Possible off-by one in batch splitting logic #13639

divega opened this issue Oct 16, 2018 · 3 comments
Assignees
Labels
closed-no-further-action The issue is closed and no further action is planned. punted-for-3.0 punted-for-6.0

Comments

@divega
Copy link
Contributor

divega commented Oct 16, 2018

While investigating workarounds for #13617, I observed that the actual max number of parameters you can have in a SqlCommand is 2098 and not 2100 as I originally believed. It turns out that is because sp_executesql takes two extra parameters: @stmt, for the SQL statement, and @params, which contains the definition of all parameters passed in @stmt.

Looking at the code in
https://github.com/aspnet/EntityFrameworkCore/blob/179a2ddaa922fdd123fb58216becdb980ef44999/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs#L23
we consider that the max number is 2100, but in
https://github.com/aspnet/EntityFrameworkCore/blob/179a2ddaa922fdd123fb58216becdb980ef44999/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs#L25
we initialize the parameter count to 1, to account only for the parameter reserved for the command text.

It is likely that the way we check with >= in
https://github.com/aspnet/EntityFrameworkCore/blob/179a2ddaa922fdd123fb58216becdb980ef44999/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs#L75
makes everything work correctly, but the code isn't clear and we expose some of these values outside of the class, so failing this issue to make sure there are not unintended consequences.

@AndriySvyryd
Copy link
Member

Related: #23281

@roji
Copy link
Member

roji commented Apr 29, 2022

Thanks @AndriySvyryd, will look at this together with #27840.

@roji
Copy link
Member

roji commented Aug 15, 2022

Confirmed that the actual limit is 2098. However, our current code looks like this:

    private const int MaxParameterCount = 2100;

    protected override bool IsValid()
        => SqlBuilder.Length < MaxScriptLength
            // A single implicit parameter for the command text itself
            && ParameterValues.Count + 1 < MaxParameterCount;

Since we add one and use less-than, I think we're fine. I'll do a small cleanup just to make this clearer.

Program to test SQL Server parameter limit
await using var conn = new SqlConnection("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false");
await conn.OpenAsync();

using var command = conn.CreateCommand();
command.CommandText =
"""
DROP TABLE IF EXISTS data;
CREATE TABLE data (f1 INT, f2 INT, f3 INT, f4 INT, f5 INT, f6 INT, f7 INT, f8 INT, f9 INT, f10 INT);
""";


for (var totalParams = 2095; totalParams < 2105; totalParams++)
{
    command.Parameters.Clear();

    var stringBuilder = new StringBuilder();
    stringBuilder.AppendLine("INSERT INTO data (f1, f2, f3, f4, f5, f6, f7, f8, f9, f10) VALUES ");

    var p = 1;
    for (var i = 0; i < 209; i++)
    {
        if (i > 0)
            stringBuilder.AppendLine(",");
        stringBuilder.Append("(");
        for (var j = 0; j < 10; j++)
        {
            if (j > 0)
                stringBuilder.Append(", ");
            stringBuilder.Append("@p" + p);
            command.Parameters.AddWithValue("p" + p++, 0);
        }
        stringBuilder.Append(")");
    }

    stringBuilder.AppendLine(";");

    for (var i = 0; i < totalParams - 209 * 10; i++)
    {
        stringBuilder.AppendLine($"INSERT INTO data (f1) VALUES (@p{p});");
        command.Parameters.AddWithValue("p" + p++, 0);
    }

    command.CommandText = stringBuilder.ToString();

    try
    {
        Console.WriteLine($"About to execute with {totalParams} parameters");
        await command.ExecuteNonQueryAsync();
        Console.WriteLine($"{totalParams} isn't too many parameters");
    }
    catch (SqlException e) when (e.ToString().Contains("The incoming request has too many parameters"))
    {
        Console.WriteLine($"{totalParams} is too many parameters");
        return;
    }
}

@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 15, 2022
@roji roji removed this from the 7.0.0 milestone Aug 15, 2022
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed type-bug closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. labels Aug 15, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2022
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. punted-for-3.0 punted-for-6.0
Projects
None yet
Development

No branches or pull requests

4 participants