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

support per query variables for CommandBehavior #1312

Merged
merged 4 commits into from
May 14, 2023

Conversation

rusher
Copy link
Contributor

@rusher rusher commented Apr 24, 2023

(Reuse part of #1304)

MariaDB and percona server permits per query variables (see https://mariadb.com/kb/en/set-statement/ and https://www.percona.com/blog/using-per-query-variable-statements-in-percona-server/)

This corresponds to commands like

SET STATEMENT <variable=value> FOR <statement>

PR is to propose using per query variable when using CommandBehavior.SchemaOnly/ CommandBehavior.SingleRow in place of multiple statements, which helps improve performance.

Benchmark results a MariaDB server:


    [Benchmark]
    public async Task<bool> ReadSingleRow()
    {
	    using (var cmd = Connection.CreateCommand())
	    {
		    cmd.CommandText = "SELECT * FROM seq_10_to_20";
		    using (var reader = await cmd.ExecuteReaderAsync(CommandBehavior.SingleRow))
		    {
			    await reader.ReadAsync();
			    reader.GetInt32(0);
			    return await reader.ReadAsync();
		    }
	    }
    }

    [Benchmark]
    public async Task<System.Data.DataColumnCollection> ReadSchemaOnly()
    {
	    using (var cmd = Connection.CreateCommand())
	    {
		    cmd.CommandText = "SELECT * FROM seq_10_to_20";
		    using (var reader = await cmd.ExecuteReaderAsync(CommandBehavior.SchemaOnly))
		    {
			    await reader.ReadAsync();
			    return reader.GetSchemaTable().Columns;
		    }
	    }
    }

results :

|         Method |        Library |     Mean |    Error |
|--------------- |--------------- |---------:|---------:|
|  ReadSingleRow | MySqlConnector | 81.15 us | 0.486 us |
| ReadSchemaOnly | MySqlConnector | 83.48 us | 0.698 us |

After PR:

|         Method |        Library |     Mean |    Error |
|--------------- |--------------- |---------:|---------:|
|  ReadSingleRow | MySqlConnector | 67.92 us | 0.569 us |
| ReadSchemaOnly | MySqlConnector | 74.44 us | 1.019 us |

(Reuse part of mysql-net#1304)

MariaDB and percona server permits per query variables
(see https://mariadb.com/kb/en/set-statement/ and https://www.percona.com/blog/using-per-query-variable-statements-in-percona-server/)

This corresponds to commands like

```
SET STATEMENT <variable=value> FOR <statement>
```

PR is to propose using per query variable when using CommandBehavior.SchemaOnly/ CommandBehavior.SingleRow in place of multiple statements, which helps improve performance.

Benchmark results a MariaDB server:

```

    [Benchmark]
    public async Task<bool> ReadSingleRow()
    {
	    using (var cmd = Connection.CreateCommand())
	    {
		    cmd.CommandText = "SELECT * FROM seq_10_to_20";
		    using (var reader = await cmd.ExecuteReaderAsync(CommandBehavior.SingleRow))
		    {
			    await reader.ReadAsync();
			    reader.GetInt32(0);
			    return await reader.ReadAsync();
		    }
	    }
    }

    [Benchmark]
    public async Task<System.Data.DataColumnCollection> ReadSchemaOnly()
    {
	    using (var cmd = Connection.CreateCommand())
	    {
		    cmd.CommandText = "SELECT * FROM seq_10_to_20";
		    using (var reader = await cmd.ExecuteReaderAsync(CommandBehavior.SchemaOnly))
		    {
			    await reader.ReadAsync();
			    return reader.GetSchemaTable().Columns;
		    }
	    }
    }
```

results :
```
|         Method |        Library |     Mean |    Error |
|--------------- |--------------- |---------:|---------:|
|  ReadSingleRow | MySqlConnector | 81.15 us | 0.486 us |
| ReadSchemaOnly | MySqlConnector | 83.48 us | 0.698 us |
```
After PR:
```
|         Method |        Library |     Mean |    Error |
|--------------- |--------------- |---------:|---------:|
|  ReadSingleRow | MySqlConnector | 67.92 us | 0.569 us |
| ReadSchemaOnly | MySqlConnector | 74.44 us | 1.019 us |
```

Signed-off-by: rusher <diego.dupin@gmail.com>
@bgrainger
Copy link
Member

I'm digging into why the tests are failing, and I think it's a MariaDB bug. What do you think?

Here's the repro code (with MySqlConnector 2.2.5; I think should work with any client).

using var connection = new MySqlConnection("server=localhost;user=root;password=pass;database=test");
connection.Open();
using var command = new MySqlCommand("""
	drop table if exists test;
	create table test(id int primary key);
	insert into test(id) values(1),(2),(3);
	""", connection);
command.ExecuteNonQuery();

// not buggy
// CountRows("SET STATEMENT sql_select_limit=1 FOR SELECT 1;");

// causes bug in next ExecuteReader: it will return 1 not 3 rows
CountRows("SET STATEMENT sql_select_limit=1 FOR SELECT 1; SET STATEMENT sql_select_limit=1 FOR SELECT 1;");

// demonstrate the bug
CountRows("SELECT id FROM test ORDER BY ID;");

void CountRows(string sql)
{
	var resultSets = 0;
	using var command = new MySqlCommand(sql, connection);
	using var reader = command.ExecuteReader();
	do
	{
		resultSets++;
		var rows = 0;
		while (reader.Read())
			rows++;
		Console.WriteLine($"Result set {resultSets}: Rows = {rows}");
	}
	while (reader.NextResult());
}

The expected output is:

Result set 1: Rows = 1
Result set 2: Rows = 1
Result set 1: Rows = 3

However, it prints:

Result set 1: Rows = 1
Result set 2: Rows = 1
Result set 1: Rows = 1

The missing rows in the result sets returned by the second invocation of ExecuteReader are causing the test failure.

@bgrainger
Copy link
Member

Added packet capture, which seems to show MariaDB 10.11 returning an incorrect resultset for the final query.

dump.zip

Comment on lines 287 to 295
var preparer = new StatementPreparer(command.CommandText!, command.RawParameters, command.CreateStatementPreparerOptions() | ((appendSemicolon || isSchemaOnly || isSingleRow) ? StatementPreparerOptions.AppendSemicolon : StatementPreparerOptions.None));
var isComplete = preparer.ParseAndBindParameters(writer);
return isComplete;
}
}
return isComplete;

var preparer1 = new StatementPreparer(command.CommandText!, command.RawParameters, command.CreateStatementPreparerOptions() | ((appendSemicolon || isSchemaOnly || isSingleRow) ? StatementPreparerOptions.AppendSemicolon : StatementPreparerOptions.None));
var isComplete1 = preparer1.ParseAndBindParameters(writer);
return isComplete1;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like some undesirable code duplication here.

Comment on lines -251 to -257
ReadOnlySpan<byte> setSqlSelectLimit0 = "SET sql_select_limit=0;\n"u8;
writer.Write(setSqlSelectLimit0);
}
else if (isSingleRow)
{
ReadOnlySpan<byte> setSqlSelectLimit1 = "SET sql_select_limit=1;\n"u8;
writer.Write(setSqlSelectLimit1);
Copy link
Member

Choose a reason for hiding this comment

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

I probably should have inlined these constants in f8a0a40#diff-15fac83b96ddd1a37bdce12382e8398f3e7cecd40ff35e6ad6a8994c5b09ab33; there's no reason to have a temporary variable.

Comment on lines 256 to 257
ReadOnlySpan<byte> setSqlSelectLimit0 = "SET sql_select_limit=0;\n"u8;
writer.Write(setSqlSelectLimit0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ReadOnlySpan<byte> setSqlSelectLimit0 = "SET sql_select_limit=0;\n"u8;
writer.Write(setSqlSelectLimit0);
writer.Write("SET sql_select_limit=0;\n"u8);

@bgrainger
Copy link
Member

The MariaDB SET STATEMENT docs aren't fully clear (to me) on whether SET STATEMENT applies to just the next statement, or to all statements in the current query.

Sending SET STATEMENT sql_select_limit=2 FOR SELECT id FROM test; SELECT id FROM test; as a single query makes it appear that the initial SET STATEMENT applies to both statements in that query. If that's intentional, then the MySqlConnector batching code should make sure that SET STATEMENT is only output once.

However, it still seems like a bug that sending two SET STATEMENTs causes one of them to apply to the next query.

@bgrainger
Copy link
Member

Changing the test app to execute these two queries:

SET STATEMENT sql_select_limit=2 FOR SELECT id FROM test; SET STATEMENT sql_select_limit=1 FOR SELECT id FROM test;
SELECT id FROM test;

produces the following output:

Result set 1: Rows = 2
Result set 2: Rows = 1
Result set 1: Rows = 2

It seems like the first SET STATEMENT is not "popped off" the execution stack and is thus still in force for the next query that's executed.

@rusher
Copy link
Contributor Author

rusher commented May 1, 2023

yes, my fault, "SET STATEMENT sql_select_limit=xx FOR" must be only set one time, so for batch, I'll correct PR (i'm not used to have multi-statement capability enable).

Still having 2 times set, having all following queries impacted is clearly a bug, i'll create an issue for MariaDB server.

rusher and others added 3 commits May 1, 2023 10:34
… variables or not.

example for command
```
using var batch = new MySqlBatch(connection)
{
	BatchCommands =
	{
		new MySqlBatchCommand("SELECT * FROM seq_1_to_5; SELECT 1"),
		new MySqlBatchCommand("SELECT * FROM seq_1_to_3"),
	},
};
```
was executing:
```
SET sql_select_limit=1;
SELECT * FROM seq_1_to_5; SELECT 1;
SET sql_select_limit=default;
SELECT * FROM seq_1_to_3;
SELECT id FROM batch_single_row ORDER BY id;
SET sql_select_limit=default;
```
now it will execute
```
SET sql_select_limit=1;
SELECT * FROM seq_1_to_5; SELECT 1;SELECT * FROM seq_1_to_3;
SET sql_select_limit=default;
```

and when using per query statement:
```
SET STATEMENT sql_select_limit=1 FOR SELECT * FROM seq_1_to_5; SELECT 1;SELECT * FROM seq_1_to_3;
```

Signed-off-by: rusher <diego.dupin@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Use UTF-8 literals to perform conversion at compile time, not runtime.

Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
@bgrainger bgrainger merged commit 881e570 into mysql-net:master May 14, 2023
23 checks passed
@bgrainger
Copy link
Member

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants