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

Reimplement MaxBatchSize as a pre-check #27696

Merged
1 commit merged into from
Mar 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/EFCore.Relational/Update/ModificationCommandBatch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,5 @@ public abstract class ModificationCommandBatch
/// <param name="cancellationToken">A <see cref="CancellationToken" /> to observe while waiting for the task to complete.</param>
/// <returns>A task that represents the asynchronous save operation.</returns>
/// <exception cref="OperationCanceledException">If the <see cref="CancellationToken" /> is canceled.</exception>
public abstract Task ExecuteAsync(
IRelationalConnection connection,
CancellationToken cancellationToken = default);
public abstract Task ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken = default);
}
30 changes: 22 additions & 8 deletions src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ public abstract class ReaderModificationCommandBatch : ModificationCommandBatch
{
private readonly List<IReadOnlyModificationCommand> _modificationCommands = new();
private readonly int _batchHeaderLength;
private readonly List<string> _pendingParameterNames = new();
private bool _requiresTransaction = true;
private int _sqlBuilderPosition, _commandResultSetCount, _resultsPositionalMappingEnabledLength;
private int _pendingParameters;

/// <summary>
/// Creates a new <see cref="ReaderModificationCommandBatch" /> instance.
Expand Down Expand Up @@ -57,6 +57,12 @@ protected ReaderModificationCommandBatch(ModificationCommandBatchFactoryDependen
/// </summary>
protected virtual IRelationalCommandBuilder RelationalCommandBuilder { get; }

/// <summary>
/// The maximum number of <see cref="ModificationCommand"/> instances that can be added to a single batch.
/// </summary>
protected virtual int MaxBatchSize
=> 1000;

/// <summary>
/// Gets the command text builder for the commands in the batch.
/// </summary>
Expand Down Expand Up @@ -98,9 +104,14 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm
throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchAlreadyComplete);
}

if (_modificationCommands.Count >= MaxBatchSize)
{
return false;
}

_sqlBuilderPosition = SqlBuilder.Length;
_commandResultSetCount = CommandResultSet.Count;
_pendingParameterNames.Clear();
_pendingParameters = 0;
_resultsPositionalMappingEnabledLength = ResultsPositionalMappingEnabled?.Length ?? 0;

AddCommand(modificationCommand);
Expand Down Expand Up @@ -144,11 +155,13 @@ protected virtual void RollbackLastCommand()
ResultsPositionalMappingEnabled.Length = _resultsPositionalMappingEnabledLength;
}

foreach (var pendingParameterName in _pendingParameterNames)
for (var i = 0; i < _pendingParameters; i++)
{
ParameterValues.Remove(pendingParameterName);
var parameterIndex = RelationalCommandBuilder.Parameters.Count - 1;
var parameter = RelationalCommandBuilder.Parameters[parameterIndex];

RelationalCommandBuilder.RemoveParameterAt(RelationalCommandBuilder.Parameters.Count - 1);
RelationalCommandBuilder.RemoveParameterAt(parameterIndex);
ParameterValues.Remove(parameter.InvariantName);
}
}

Expand All @@ -173,7 +186,8 @@ protected virtual void SetRequiresTransaction(bool requiresTransaction)
/// Checks whether the command text is valid.
/// </summary>
/// <returns><see langword="true" /> if the command text is valid; <see langword="false" /> otherwise.</returns>
protected abstract bool IsValid();
protected virtual bool IsValid()
=> true;

/// <summary>
/// Adds Updates the command text for the command at the given position in the <see cref="ModificationCommands" /> list.
Expand Down Expand Up @@ -265,7 +279,7 @@ protected virtual void AddParameter(IColumnModification columnModification)

ParameterValues.Add(columnModification.ParameterName, columnModification.Value);

_pendingParameterNames.Add(columnModification.ParameterName);
_pendingParameters++;
}

if (columnModification.UseOriginalValueParameter)
Expand All @@ -278,7 +292,7 @@ protected virtual void AddParameter(IColumnModification columnModification)

ParameterValues.Add(columnModification.OriginalParameterName, columnModification.OriginalValue);

_pendingParameterNames.Add(columnModification.OriginalParameterName);
_pendingParameters++;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@ public SingularModificationCommandBatch(ModificationCommandBatchFactoryDependenc
}

/// <summary>
/// Returns <see langword="true" /> only when the batch contains a single command.
/// The maximum number of <see cref="ModificationCommand"/> instances that can be added to a single batch; always returns 1.
/// </summary>
/// <returns>
/// <see langword="true" /> when the batch contains a single command, <see langword="false" /> otherwise.
/// </returns>
protected override bool IsValid()
=> ModificationCommands.Count == 1;
protected override int MaxBatchSize
=> 1;
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ public class SqlServerModificationCommandBatch : AffectedCountModificationComman
private const int DefaultNetworkPacketSizeBytes = 4096;
private const int MaxScriptLength = 65536 * DefaultNetworkPacketSizeBytes / 2;
private const int MaxParameterCount = 2100;
private const int MaxRowCount = 1000;
private readonly int _maxBatchSize;
private readonly List<IReadOnlyModificationCommand> _pendingBulkInsertCommands = new();

Expand All @@ -30,16 +29,9 @@ public class SqlServerModificationCommandBatch : AffectedCountModificationComman
/// </summary>
public SqlServerModificationCommandBatch(
ModificationCommandBatchFactoryDependencies dependencies,
int? maxBatchSize)
int maxBatchSize)
: base(dependencies)
{
if (maxBatchSize is <= 0)
{
throw new ArgumentOutOfRangeException(nameof(maxBatchSize), RelationalStrings.InvalidMaxBatchSize(maxBatchSize.Value));
}

_maxBatchSize = Math.Min(maxBatchSize ?? 42, MaxRowCount);
}
=> _maxBatchSize = maxBatchSize;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -50,6 +42,15 @@ public SqlServerModificationCommandBatch(
protected new virtual ISqlServerUpdateSqlGenerator UpdateSqlGenerator
=> (ISqlServerUpdateSqlGenerator)base.UpdateSqlGenerator;

/// <summary>
/// The maximum number of <see cref="ModificationCommand"/> instances that can be added to a single batch.
/// </summary>
/// <remarks>
/// For SQL Server, this is 42 by default, and cannot exceed 1000.
/// </remarks>
protected override int MaxBatchSize
=> _maxBatchSize;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand All @@ -74,8 +75,7 @@ protected override void RollbackLastCommand()
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override bool IsValid()
=> ModificationCommands.Count <= _maxBatchSize
&& SqlBuilder.Length < MaxScriptLength
=> SqlBuilder.Length < MaxScriptLength
// A single implicit parameter for the command text itself
&& ParameterValues.Count + 1 < MaxParameterCount;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Update.Internal;
/// </summary>
public class SqlServerModificationCommandBatchFactory : IModificationCommandBatchFactory
{
private readonly IDbContextOptions _options;
private const int DefaultMaxBatchSize = 42;
private const int MaxMaxBatchSize = 1000;
private readonly int _maxBatchSize;

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -26,7 +28,16 @@ public SqlServerModificationCommandBatchFactory(
IDbContextOptions options)
{
Dependencies = dependencies;
_options = options;

_maxBatchSize = Math.Min(
options.Extensions.OfType<SqlServerOptionsExtension>().FirstOrDefault()?.MaxBatchSize ?? DefaultMaxBatchSize,
MaxMaxBatchSize);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd we may want to start throwing if the MaxMaxBatchSize is exceeded, rather than silently reducing it (after all the user explicitly wants something else). Would be a pretty minor breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't worry about that. With the current way if MaxMaxBatchSize changes that user just needs to upgrade

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was to either allow users to explicitl go beyond the 1000 MaxMaxBatchSize (it's their responsibility...), or to throw if they try to do so, rather than silently reduce to 1000.. But not very important.


if (_maxBatchSize <= 0)
{
throw new ArgumentOutOfRangeException(
nameof(RelationalOptionsExtension.MaxBatchSize), RelationalStrings.InvalidMaxBatchSize(_maxBatchSize));
}
}

/// <summary>
Expand All @@ -41,9 +52,5 @@ public SqlServerModificationCommandBatchFactory(
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual ModificationCommandBatch Create()
{
var optionsExtension = _options.Extensions.OfType<SqlServerOptionsExtension>().FirstOrDefault();

return new SqlServerModificationCommandBatch(Dependencies, optionsExtension?.MaxBatchSize);
}
=> new SqlServerModificationCommandBatch(Dependencies, _maxBatchSize);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,11 @@ namespace Microsoft.EntityFrameworkCore.TestUtilities;

public class TestModificationCommandBatch : SingularModificationCommandBatch
{
private readonly int _maxBatchSize;

public TestModificationCommandBatch(
ModificationCommandBatchFactoryDependencies dependencies,
int? maxBatchSize)
: base(dependencies)
{
_maxBatchSize = maxBatchSize ?? 1;
}
=> MaxBatchSize = maxBatchSize ?? 1;

protected override bool IsValid()
=> ModificationCommands.Count <= _maxBatchSize;
protected override int MaxBatchSize { get; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ public void AddCommand_does_not_add_command_batch_is_invalid()
",
batch.CommandText,
ignoreLineEndingDifferences: true);

Assert.Equal(1, batch.StoreCommand.RelationalCommand.Parameters.Count);
Assert.Equal(1, batch.StoreCommand.ParameterValues.Count);
}

[ConditionalFact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ private class FakeDbContext : DbContext
{
}

private static TestSqlServerModificationCommandBatch CreateBatch(int? maxBatchSize = null)
private static TestSqlServerModificationCommandBatch CreateBatch(int maxBatchSize = 42)
{
var typeMapper = CreateTypeMappingSource();

Expand Down Expand Up @@ -100,7 +100,7 @@ private static IModificationCommand CreateModificationCommand(

private class TestSqlServerModificationCommandBatch : SqlServerModificationCommandBatch
{
public TestSqlServerModificationCommandBatch(ModificationCommandBatchFactoryDependencies dependencies, int? maxBatchSize)
public TestSqlServerModificationCommandBatch(ModificationCommandBatchFactoryDependencies dependencies, int maxBatchSize)
: base(dependencies, maxBatchSize)
{
}
Expand Down