Skip to content

Clamp RecordsAffected to int.MaxValue instead of throwing #28

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.

Sign up for GitHub

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

Open
francoisjehl wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clamp RecordsAffected to int.MaxValue instead of throwing #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4576eb6
fix: clamp RecordsAffected to int.MaxValue instead of throwing Overfl…
francoisjehl Oct 14, 2026
2e76d90
fix: expose RealRecordsAffected and reuse WriteOk in fake server
francoisjehl Oct 16, 2026
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
8 changes: 6 additions & 2 deletions src/SingleStoreConnector/SingleStoreCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ private SingleStoreCommand(SingleStoreCommand other)
/// <returns>The number of rows affected.</returns>
/// <remarks>For UPDATE, INSERT, and DELETE statements, the return value is the number of rows affected by the command.
/// For stored procedures, the return value is the number of rows affected by the last statement in the stored procedure,
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.</remarks>
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.
/// If the number of affected rows exceeds <see cref="int.MaxValue"/>, the return value is <see cref="int.MaxValue"/>;

@okramarenko okramarenko Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for updating these!

one detail though: ExecuteNonQuery() creates and disposes the reader internally, so users can't access RealRecordsAffected. could we add this info here and in ExecuteNonQueryAsync()? smth like:

If the number of affected rows exceeds int.MaxValue, the return value is int.MaxValue. To obtain the exact count, use ExecuteReader(), call SingleStoreDataReader.NextResult until it returns false, then read SingleStoreDataReader.RealRecordsAffected

for the async version we should use ExecuteReaderAsync() and NextResultAsync()

/// use <see cref="SingleStoreDataReader.RealRecordsAffected"/> to get the exact count.</remarks>
public override int ExecuteNonQuery() => ExecuteNonQueryAsync(IOBehavior.Synchronous, CancellationToken.None).Result;

public override object? ExecuteScalar() => ExecuteScalarAsync(IOBehavior.Synchronous, CancellationToken.None).Result;
Expand Down Expand Up @@ -292,7 +294,9 @@ protected override DbDataReader ExecuteDbDataReader(CommandBehavior behavior) =>
/// <returns>A task representing the asynchronous operation.</returns>
/// <remarks>For UPDATE, INSERT, and DELETE statements, the return value is the number of rows affected by the command.
/// For stored procedures, the return value is the number of rows affected by the last statement in the stored procedure,
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.</remarks>
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.
/// If the number of affected rows exceeds <see cref="int.MaxValue"/>, the return value is <see cref="int.MaxValue"/>;
/// use <see cref="SingleStoreDataReader.RealRecordsAffected"/> to get the exact count.</remarks>
public override Task<int> ExecuteNonQueryAsync(CancellationToken cancellationToken) =>
ExecuteNonQueryAsync(AsyncIOBehavior, cancellationToken).AsTask();

Expand Down
12 changes: 9 additions & 3 deletions src/SingleStoreConnector/SingleStoreDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,10 @@ public override bool HasRows
/// </summary>
/// <remarks>For UPDATE, INSERT, and DELETE statements, the return value is the number of rows affected by the command.
/// For stored procedures, the return value is the number of rows affected by the last statement in the stored procedure,
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.</remarks>
public override int RecordsAffected => RealRecordsAffected is ulong recordsAffected ? checked((int) recordsAffected) : -1;
/// or zero if the last statement is a SELECT. For all other types of statements, the return value is -1.
/// If the number of affected rows exceeds <see cref="int.MaxValue"/>, the return value is <see cref="int.MaxValue"/>;
/// use <see cref="RealRecordsAffected"/> to get the exact count.</remarks>
public override int RecordsAffected => RealRecordsAffected is ulong recordsAffected ? (int) Math.Min(recordsAffected, int.MaxValue) : -1;

@okramarenko okramarenko Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for the fix!

one thing to keep in mind is that previously the OverflowException made the caller aware that the count didn't fit into int, now there's no way a caller who gets int.MaxValue can't tell whether they actually did get int.MaxValue or more.

that's why I think it makes sense to make RealRecordsAffected public with internal set and add a mention of it in the remarks, so the users know where to look for it.

got this idea from Npsql connector -- https://github.com/npgsql/npgsql/blob/af6676e70ea51699df13b9f97db8f02bb9b6f629/src/Npgsql/NpgsqlDataReader.cs#L885-L905

@francoisjehl francoisjehl Oct 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're welcome and good call!

public override int GetOrdinal(string name) => GetResultSet().GetOrdinal(name);

public override bool GetBoolean(int ordinal) => GetResultSet().GetCurrentRow().GetBoolean(ordinal);
Expand Down Expand Up @@ -463,7 +465,11 @@ protected override void Dispose(bool disposing)
internal Activity? Activity { get; private set; }
internal ISingleStoreCommand? Command { get; private set; }
internal SingleStoreConnection? Connection => Command?.Connection;
internal ulong? RealRecordsAffected { get; set; }
/// <summary>
/// Gets the number of rows changed, inserted, or deleted by execution of the SQL statement, without truncation to <see cref="int"/>.
/// </summary>
/// <remarks>Returns <c>null</c> when <see cref="RecordsAffected"/> would return -1.</remarks>
public ulong? RealRecordsAffected { get; internal set; }
internal ServerSession? Session => Command?.Connection!.Session;

internal async Task InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary<string, CachedProcedure?>? cachedProcedures, ISingleStoreCommand command, CommandBehavior behavior, Activity? activity, IOBehavior ioBehavior, CancellationToken cancellationToken)
Expand Down
12 changes: 12 additions & 0 deletions tests/SingleStoreConnector.Tests/ConnectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ public void Dispose()
m_server.Stop();
}

[Fact]
public void RecordsAffectedAboveInt32MaxValueIsClamped()
{
using var connection = new SingleStoreConnection(m_csb.ConnectionString);
connection.Open();
using var command = new SingleStoreCommand("ALTER TABLE big_table ADD COLUMN c INT", connection);
Assert.Equal(int.MaxValue, command.ExecuteNonQuery());
using var reader = command.ExecuteReader();
Assert.Equal(int.MaxValue, reader.RecordsAffected);
Assert.Equal(4_000_000_000UL, reader.RealRecordsAffected);
}

[Fact]
public void PooledConnectionIsReturnedToPool()
{
Expand Down
18 changes: 16 additions & 2 deletions tests/SingleStoreConnector.Tests/FakeSingleStoreServerConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ public async Task RunAsync(TcpClient client, CancellationToken token)
var wasSet = CancelQueryEvent.Wait(0, token);
await SendAsync(stream, 1, WriteOk);
}
else if (query == "ALTER TABLE big_table ADD COLUMN c INT")

@okramarenko okramarenko Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this duplicates WriteOk except for the value of affected-rows field. an optional ulong affectedRows = 0 parameter in WriteOk might be a better choice to avoid duplications

@francoisjehl francoisjehl Oct 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

WriteOk is passed as a method group in several places, so an optional parameter didn't want to compile.
Thus I thought an overload would do the job? You tell me.

@okramarenko okramarenko Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good point! optional parameters aren't considered in method group conversion, so an overload
is the right call. lets have WriteOk(BinaryWriter) delgating to new WriteOk(BinaryWriter, ulong affectedRows)

{
await SendAsync(stream, 1, x => WriteOk(x, 4_000_000_000UL));
}
else if (query == "select infinity")
{
var packets = new[]
Expand Down Expand Up @@ -295,10 +299,20 @@ private void WriteInitialHandshake(BinaryWriter writer)
writer.Write((byte) 0);
}

private static void WriteOk(BinaryWriter writer)
private static void WriteOk(BinaryWriter writer) => WriteOk(writer, 0);

private static void WriteOk(BinaryWriter writer, ulong affectedRows)
{
writer.Write((byte) 0); // signature
writer.Write((byte) 0); // 0 rows affected
if (affectedRows < 251)
{
writer.Write((byte) affectedRows); // rows affected
}
else
{
writer.Write((byte) 0xFE); // 8-byte length-encoded integer
writer.Write(affectedRows); // rows affected
}
writer.Write((byte) 0); // last insert ID
writer.Write((ushort) 0); // server status
writer.Write((ushort) 0); // warning count
Expand Down