-
Notifications
You must be signed in to change notification settings - Fork 335
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
Permit skipping metadata for MariaDB #1301
Conversation
Signed-off-by: rusher <[email protected]>
Since MariaDB 10.6 (with https://jira.mariadb.org/browse/MDEV-19237) binary result-set skip sending metadata when they haven't changed. This avoid network data and parsing client side. Benchmarks results using DDL: ``` CREATE TABLE test100 (i1 int,i2 int,...,i100 int); INSERT INTO test100 value (1,2,...,100); ``` test methods: ``` [Benchmark] public async Task<int> readOneRow100FieldsAsync() { var total = 0; using (var cmd = Connection.CreateCommand()) { cmd.CommandText = "select * FROM test100"; using (var reader = await cmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) for (var i = 0; i < 100; i++) total += reader.GetInt32(i); } } return total; } [Benchmark] public async Task<int> readOneRow100FieldsPrepareAsync() { var total = 0; using (var cmd = Connection.CreateCommand()) { cmd.CommandText = "select * FROM test100"; cmd.Prepare(); using (var reader = await cmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) for (var i = 0; i < 100; i++) total += reader.GetInt32(i); } } return total; } ``` before PR: ``` | Method | Library | Mean | Error | |-------------------------------- |--------------- |----------:|---------:| | readOneRow100FieldsAsync | MySqlConnector | 104.47 us | 1.468 us | | readOneRow100FieldsPrepareAsync | MySqlConnector | 92.49 us | 1.154 us | ``` After PR: ``` | Method | Library | Mean | Error | |-------------------------------- |--------------- |----------:|---------:| | readOneRow100FieldsAsync | MySqlConnector | 103.99 us | 0.914 us | | readOneRow100FieldsPrepareAsync | MySqlConnector | 71.50 us | 1.402 us | ``` Signed-off-by: rusher <[email protected]>
Thanks! I will take a look when I'm back from vacation. I assume this may be related to #446 although MariaDB uses a different flag value to enable the capability? (Or maybe it's a completely different on-the-wire way to avoid sending metadata?) |
In terms of the protocol, this corresponds to the mysql change you indicate. The MySQL implementation is a very specific optimization for some customers that cache application-side metadata, pushing the metadata to the connector metadata for each request. For MySQL, there is a CLIENT_OPTIONAL_RESULTSET_METADATA capability that enables this feature, then when @@session.resultset_metadata is "NONE", then all result set metadata will not be sent. Upon receiving the query result, the application provides metadata to the connector to parse the rows. MariaDB's implementation is more general: when the client sets the MARIADB_CLIENT_CACHE_METADATA capability, the metadata MAY not be sent: running a COM_QUERY will still have the metadata. The modification concerns only the prepared command (COM_STMT_EXECUTE). Once prepared, the connector already knows the metadata, so on the second run, when MARIADB_CLIENT_CACHE_METADATA is set, the server will not return the metadata to the COM_STMT_EXECUTE result set if the metadata has not changed since the previous run. So to sum up, the first run result will have metadata, the second one won't. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the MariaDbCacheMetadata
value is defined incorrectly and needs to be fixed.
/// <summary> | ||
/// Associated prepared statements of commands | ||
/// </summary> | ||
public PreparedStatements? PreparedStatements; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: this struct is starting to accumulate a lot of mutable public fields; redesign?
{ | ||
ColumnTypes[column] = TypeMapper.ConvertToMySqlDbType(ColumnDefinitions[column], | ||
Connection.TreatTinyAsBoolean, Connection.GuidFormat); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Can we move this outside the if
statement (and remove the corresponding code from the else
branch)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think best would be to get rid of the ColumnTypes variable, saving some calculation that are useful for metadata only, so that may not be used at all. => save ConnectionSettings and just rely on ColumnDefinitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed: 5f72a2f.
/// https://mariadb.com/kb/en/result-set-packets/#column-count-packet | ||
/// Packet contains a columnCount, and - if capability MARIADB_CLIENT_CACHE_METADATA is set - a flag to indicate if metadata follows | ||
/// </summary> | ||
internal sealed class ColumnCountPayload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Most other Payload
objects are class
es, but would it be worthwhile making this (and others?) a readonly struct
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since MariaDB 10.6 (with https://jira.mariadb.org/browse/MDEV-19237) binary result-set skip sending metadata when they haven't changed. This avoid network data and parsing client side. Benchmarks results using DDL: ``` CREATE TABLE test100 (i1 int,i2 int,...,i100 int); INSERT INTO test100 value (1,2,...,100); ``` test methods: ``` [Benchmark] public async Task<int> readOneRow100FieldsAsync() { var total = 0; using (var cmd = Connection.CreateCommand()) { cmd.CommandText = "select * FROM test100"; using (var reader = await cmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) for (var i = 0; i < 100; i++) total += reader.GetInt32(i); } } return total; } [Benchmark] public async Task<int> readOneRow100FieldsPrepareAsync() { var total = 0; using (var cmd = Connection.CreateCommand()) { cmd.CommandText = "select * FROM test100"; cmd.Prepare(); using (var reader = await cmd.ExecuteReaderAsync()) { while (await reader.ReadAsync()) for (var i = 0; i < 100; i++) total += reader.GetInt32(i); } } return total; } ``` before PR: ``` | Method | Library | Mean | Error | |-------------------------------- |--------------- |----------:|---------:| | readOneRow100FieldsAsync | MySqlConnector | 104.47 us | 1.468 us | | readOneRow100FieldsPrepareAsync | MySqlConnector | 92.49 us | 1.154 us | ``` After PR: ``` | Method | Library | Mean | Error | |-------------------------------- |--------------- |----------:|---------:| | readOneRow100FieldsAsync | MySqlConnector | 103.99 us | 0.914 us | | readOneRow100FieldsPrepareAsync | MySqlConnector | 71.50 us | 1.402 us | ``` Signed-off-by: rusher <[email protected]>
# Conflicts: # src/MySqlConnector/Protocol/ProtocolCapabilities.cs
Signed-off-by: rusher <[email protected]>
Since MariaDB 10.6 (with https://jira.mariadb.org/browse/MDEV-19237) binary result-set can skip sending metadata when they haven't changed.
This avoid network data and parsing client side.
Benchmarks results using DDL:
test methods:
before PR:
After PR: