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.

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

Jump to bottom

Avoids SET NAMES commands if not needed #1494

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Avoids SET NAMES commands if not needed #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/MySqlConnector/Core/ConcatenatedCommandPayloadCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public bool WriteQueryCommand(ref CommandListPosition commandListPosition, IDict

// ConcatenatedCommandPayloadCreator is only used by MySqlBatch, and MySqlBatchCommand doesn't expose attributes,
// so just write an empty attribute set if the server needs it.
if (commandListPosition.CommandAt(commandListPosition.CommandIndex).Connection!.Session.SupportsQueryAttributes)
if (commandListPosition.CommandAt(commandListPosition.CommandIndex).Connection!.Session.Context.SupportsQueryAttributes)
{
// attribute count
writer.WriteLengthEncodedInteger(0);
Expand Down
2 changes: 1 addition & 1 deletion src/MySqlConnector/Core/ConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public async ValueTask<ServerSession> GetSessionAsync(MySqlConnection connection
}
else
{
if (ConnectionSettings.ConnectionReset || session.DatabaseOverride is not null)
if (ConnectionSettings.ConnectionReset || !session.Context.IsInitialDatabase())
{
if (timeoutMilliseconds != 0)
session.SetTimeout(Math.Max(1, timeoutMilliseconds - Utility.GetElapsedMilliseconds(startingTimestamp)));
Expand Down
29 changes: 29 additions & 0 deletions src/MySqlConnector/Core/Context.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using MySqlConnector.Protocol;

namespace MySqlConnector.Core;

internal sealed class Context
{
public Context(ProtocolCapabilities protocolCapabilities, string? database, int connectionId)
{
SupportsDeprecateEof = (protocolCapabilities & ProtocolCapabilities.DeprecateEof) != 0;
SupportsCachedPreparedMetadata = (protocolCapabilities & ProtocolCapabilities.MariaDbCacheMetadata) != 0;
SupportsQueryAttributes = (protocolCapabilities & ProtocolCapabilities.QueryAttributes) != 0;
SupportsSessionTrack = (protocolCapabilities & ProtocolCapabilities.SessionTrack) != 0;
ConnectionId = connectionId;
Database = database;
m_initialDatabase = database;
}

public bool SupportsDeprecateEof { get; }
public bool SupportsQueryAttributes { get; }
public bool SupportsSessionTrack { get; }
public bool SupportsCachedPreparedMetadata { get; }
public string? ClientCharset { get; set; }

public string? Database { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I see the appeal in moving the connection-context-specific data into this new class, but I think it makes it harder to understand to have state spread across ServerSession and now Context. I'd rather this class be an immutable collection of data that's passed into methods that need to query the current capabilities.

I also think it's a little "unpredictable" to have these properties updated without any notification that it's happening. (The old way wasn't great, either, in that code had to read the OkPayload.NewSchema and act on it, and if it didn't, that was a bug.) I also don't like the OkPayload parsing code having the side-effect of updating this mutable state, instead of just being a property-bag/DTO of all the data that was read out of the OK packet.

Made it immutable here: 77ad191

private readonly string? m_initialDatabase;
public bool IsInitialDatabase() => string.Equals(m_initialDatabase, Database, StringComparison.Ordinal);

public int ConnectionId { get; set; }
}
16 changes: 7 additions & 9 deletions src/MySqlConnector/Core/ResultSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public async Task ReadResultSetHeaderAsync(IOBehavior ioBehavior)
var firstByte = payload.HeaderByte;
if (firstByte == OkPayload.Signature)
{
var ok = OkPayload.Create(payload.Span, Session.SupportsDeprecateEof, Session.SupportsSessionTrack);
var ok = OkPayload.Create(payload.Span, Session.Context);
Copy link
Member

Choose a reason for hiding this comment

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

I like bundling these together, but it could be done using a new interface that ServerSession implements. That would avoid allocating a second object every time a ServerSession is created. Changed here: 8466783.


// if we've read a result set header then this is a SELECT statement, so we shouldn't overwrite RecordsAffected
// (which should be -1 for SELECT) unless the server reports a non-zero count
Expand All @@ -48,8 +48,6 @@ public async Task ReadResultSetHeaderAsync(IOBehavior ioBehavior)
if (ok.LastInsertId != 0)
Command?.SetLastInsertedId((long) ok.LastInsertId);
WarningCount = ok.WarningCount;
if (ok.NewSchema is not null)
Connection.Session.DatabaseOverride = ok.NewSchema;
m_columnDefinitions = default;
State = (ok.ServerStatus & ServerStatus.MoreResultsExist) == 0
? ResultSetState.NoMoreData
Expand Down Expand Up @@ -109,7 +107,7 @@ public async Task ReadResultSetHeaderAsync(IOBehavior ioBehavior)
}
else
{
var columnCountPacket = ColumnCountPayload.Create(payload.Span, Session.SupportsCachedPreparedMetadata);
var columnCountPacket = ColumnCountPayload.Create(payload.Span, Session.Context.SupportsCachedPreparedMetadata);
var columnCount = columnCountPacket.ColumnCount;
if (!columnCountPacket.MetadataFollows)
{
Expand All @@ -132,7 +130,7 @@ public async Task ReadResultSetHeaderAsync(IOBehavior ioBehavior)
m_columnDefinitions = m_columnDefinitionPayloadCache.AsMemory(0, columnCount);

// if the server supports metadata caching but has re-sent it, something has changed since last prepare/execution and we need to update the columns
var preparedColumns = Session.SupportsCachedPreparedMetadata ? DataReader.LastUsedPreparedStatement?.Columns : null;
var preparedColumns = Session.Context.SupportsCachedPreparedMetadata ? DataReader.LastUsedPreparedStatement?.Columns : null;

for (var column = 0; column < columnCount; column++)
{
Expand All @@ -156,7 +154,7 @@ public async Task ReadResultSetHeaderAsync(IOBehavior ioBehavior)
}
}

if (!Session.SupportsDeprecateEof)
if (!Session.Context.SupportsDeprecateEof)
{
payload = await Session.ReceiveReplyAsync(ioBehavior, CancellationToken.None).ConfigureAwait(false);
_ = EofPayload.Create(payload.Span);
Expand Down Expand Up @@ -252,13 +250,13 @@ public async Task<bool> ReadAsync(IOBehavior ioBehavior, CancellationToken cance

if (payload.HeaderByte == EofPayload.Signature)
{
if (Session.SupportsDeprecateEof && OkPayload.IsOk(payload.Span, Session.SupportsDeprecateEof))
if (Session.Context.SupportsDeprecateEof && OkPayload.IsOk(payload.Span, Session.Context))
{
var ok = OkPayload.Create(payload.Span, Session.SupportsDeprecateEof, Session.SupportsSessionTrack);
var ok = OkPayload.Create(payload.Span, Session.Context);
BufferState = (ok.ServerStatus & ServerStatus.MoreResultsExist) == 0 ? ResultSetState.NoMoreData : ResultSetState.HasMoreData;
return null;
}
if (!Session.SupportsDeprecateEof && EofPayload.IsEof(payload))
if (!Session.Context.SupportsDeprecateEof && EofPayload.IsEof(payload))
{
var eof = EofPayload.Create(payload.Span);
BufferState = (eof.ServerStatus & ServerStatus.MoreResultsExist) == 0 ? ResultSetState.NoMoreData : ResultSetState.HasMoreData;
Expand Down
Loading