Permit real prepared stored procedure #1314

rusher · 2023-04-28T12:18:34Z

When using BINARY protocol, output parameters are sent using an additional result-set that is marked with intermediate EOF and ending EOF/OK Packet with flag SERVER_PS_OUT_PARAMS set (Capability PS_MULTI_RESULTS has been added to permit that).

This is an old functionality, working for any type of server > 5.0.

I think that using binary implementation must even be the basis : stored procedure can always be prepared, and only BINARY protocol permit to get output parameter.

This permits better performance than using multi-resultset with "SET @inParam1=xx;...; SELECT @outParam1,@outParam2".

Since implementation use streaming result-set, DEPRECATE_EOF capability is disabled, in order to identify "output parameter resultset" in intermediate EOF. It could be improved checking if next packet is a row packet and then fetching next packet to check if this is an EOF/OK_Packet with SERVER_PS_OUT_PARAMS flag set, but i didn't know how to make that properly.

bench base on StoredProcedureCircle test, like

using (var cmd = Connection.CreateCommand())
		{
			cmd.CommandText = "circle";
			cmd.CommandType = CommandType.StoredProcedure;
			cmd.Parameters.Add(new MySqlParameter()
			{
				ParameterName = "@radius",
				DbType = DbType.Double,
				Direction = ParameterDirection.Input,
				Value = 1.0,
			});
			...
			cmd.Parameters.Add(new MySqlParameter()
			{
				ParameterName = "@shape",
				DbType = DbType.String,
				Direction = ParameterDirection.Output,
			});
			using (var reader = await cmd.ExecuteReaderAsync())
			{
				if (await reader.ReadAsync())
					reader.GetValue(0);
			}

		}

show improved result:
with 2.3.0-beta.2:

 |                 Method |        Library |     Mean |   Error |
|----------------------- |--------------- |---------:|--------:|
|     storeProcedureText | MySqlConnector | 120.9 us | 2.41 us |

with PR:

|                 Method |        Library |      Mean |    Error |
|----------------------- |--------------- |----------:|---------:|
|     storeProcedureText | MySqlConnector | 118.79 us | 2.311 us |
| storeProcedurePrepared | MySqlConnector |  93.31 us | 1.021 us |

@radius

When using BINARY protocol, output parameters are sent using an additional result-set that is marked with intermediate EOF and ending EOF/OK Packet with flag [SERVER_PS_OUT_PARAMS](https://mariadb.com/kb/en/ok_packet/#server-status-flag) set (Capability PS_MULTI_RESULTS has been added to permit that). This is an old functionality, working for any type of server > 5.0. I'm even wondering if using binary cannot be used in all situations, because stored procedure can always be prepared, and only BINARY protocol permit to get output parameter. This permits better performance than using multi-resultset with "SET @inParam1=xx;...; SELECT @outParam1,@outParam2". Since implementation use streaming result-set, DEPRECATE_EOF capability is disabled, in order to identify "output parameter resultset" in intermediate EOF. It could be improved checking if next packet is a row packet and then fetching next packet to check if this is an EOF/OK_Packet with SERVER_PS_OUT_PARAMS flag set, but i didn't know how to make that properly. bench base on StoredProcedureCircle test, like ``` using (var cmd = Connection.CreateCommand()) { cmd.CommandText = "circle"; cmd.CommandType = CommandType.StoredProcedure; cmd.Parameters.Add(new MySqlParameter() { ParameterName = "@radius", DbType = DbType.Double, Direction = ParameterDirection.Input, Value = 1.0, }); ... cmd.Parameters.Add(new MySqlParameter() { ParameterName = "@shape", DbType = DbType.String, Direction = ParameterDirection.Output, }); using (var reader = await cmd.ExecuteReaderAsync()) { if (await reader.ReadAsync()) reader.GetValue(0); } } ``` show improved result: with 2.3.0-beta.2: ``` | Method | Library | Mean | Error | |----------------------- |--------------- |---------:|--------:| | storeProcedureText | MySqlConnector | 120.9 us | 2.41 us | ``` with PR: ``` | Method | Library | Mean | Error | |----------------------- |--------------- |----------:|---------:| | storeProcedureText | MySqlConnector | 118.79 us | 2.311 us | | storeProcedurePrepared | MySqlConnector | 93.31 us | 1.021 us | ``` Signed-off-by: rusher <diego.dupin@gmail.com>

bgrainger · 2023-04-29T02:05:11Z

src/MySqlConnector/Protocol/Payloads/HandshakeResponse41Payload.cs

-	private static ByteBufferWriter CreateCapabilitiesPayload(ProtocolCapabilities serverCapabilities, ConnectionSettings cs, bool useCompression, CharacterSet characterSet, ProtocolCapabilities additionalCapabilities = 0)
-	{
-		var writer = new ByteBufferWriter();
-
-		var clientCapabilities = (ProtocolCapabilities.Protocol41 |
+	public static ProtocolCapabilities CreateClientCapabilities(ProtocolCapabilities serverCapabilities, ConnectionSettings cs, bool useCompression, ProtocolCapabilities additionalCapabilities = 0) =>
+		(ProtocolCapabilities.Protocol41 |


I can't tell if this was a required change, or just a refactoring done along the way?

Either way, some change here seems to be causing a "Bad handshake" error.

bgrainger · 2023-04-29T02:08:38Z

src/MySqlConnector/Core/ServerSession.cs

-				m_useCompression = cs.UseCompression && (initialHandshake.ProtocolCapabilities & ProtocolCapabilities.Compress) != 0;
+				clientCapabilities =
+					HandshakeResponse41Payload.CreateClientCapabilities(initialHandshake.ProtocolCapabilities, cs, m_useCompression,
+						ProtocolCapabilities.Ssl);


Not sure that it's correct to advertise ProtocolCapabilities.Ssl here if a SSL connection may not be attempted.

yes, this is a bug

bgrainger · 2023-04-29T02:34:30Z

src/MySqlConnector/Protocol/Payloads/HandshakeResponse41Payload.cs

-		                          ProtocolCapabilities.DeprecateEof |
+
+		                          // ProtocolCapabilities.DeprecateEof |


Not sure that this tradeoff is worth the benefit; the EOF packet has been deprecated since MySQL 5.7.5, and this enables CLIENT_SESSION_TRACK, etc.

bgrainger · 2023-04-29T14:51:52Z

src/MySqlConnector/MySqlParameter.cs

@@ -12,6 +12,8 @@ namespace MySqlConnector;

 public sealed class MySqlParameter : DbParameter, IDbDataParameter, ICloneable
 {
+	public static readonly MySqlParameter NullParameter = new MySqlParameter("", null);


Avoid creating a public static property with a value that's a mutable object. User code could run MySqlParameter.NullParameter.Value = 42.

correct, i'll correct that

bgrainger · 2023-04-29T14:55:30Z

src/MySqlConnector/Core/CachedProcedure.cs

+			cmd.Parameters.Add(new() { Value = schema });
+			cmd.Parameters.Add(new() { Value = component });
+			cmd.Parameters.Add(new() { Value = schema });
+			cmd.Parameters.Add(new() { Value = component });


Are the changes in this file required for this PR, or just an optimisation (to reuse this command server-side) that seemed desirable?

Additionally, I think it should be unnecessary to switch from named to positional parameters (and double the parameter count), just because you're calling Prepare. The parameter binding code should figure that out IIRC.

I think this might actually be a "pessimisation"; since there are two statements, two COM_EXECUTE_STMT payloads will have to be sent (instead of one COM_QUERY), so there will be multiple DB roundtrips. A quick benchmark bears that out:

Method Mean Error StdDev Allocated

Normal 3.190 ms 0.0975 ms 0.2766 ms 3.43 KB

Prepared 3.983 ms 0.0783 ms 0.1490 ms 3.08 KB

this change can really be changed into just "cmd.Prepare();".
About "pessimisation", this isn't exact: the first stored procedure will indeed have a PREPARE + EXECUTE command that will be slower that a COM_QUERY command, but each time another stored procedure command will be called, only an EXECUTE command will be issued, and this will be faster (especially when using mariadb now that metadata skip is implemented). I hesitate to improve PREPARE + EXECUTE in the same PR, but this will be in another PR : MariaDB permit pipelining PREPARE + EXECUTE command, EXECUTE using statement id 0xffffffff meaning using last prepared statement, this permits to have nearly the same execution time than a COM_QUERY, but next execution being just an EXECUTE command will then be way better.

bgrainger

It seems like this PR mixes multiple refactorings (CachedProcedure, HandshakeResponse41Payload) with the actual functional change being made. Please limit the PR to the minimum code necessary to accomplish the change, and open additional PRs for the other changes. (Or structure it as a clear sequence of independent commits that can be reviewed independently.)

While you noted that this functionality is "working for any type of server > 5.0", I wonder if this code should be gated by a test for ProtocolCapabilities.PreparedStatementMultiResults to help compatibility with non-MySQL servers that speak the MySQL protocol?

bgrainger reviewed Apr 29, 2023

View reviewed changes

bgrainger requested changes Apr 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit real prepared stored procedure #1314

Permit real prepared stored procedure #1314

rusher commented Apr 28, 2023

bgrainger Apr 29, 2023

bgrainger Apr 29, 2023

rusher May 1, 2023

bgrainger Apr 29, 2023

bgrainger Apr 29, 2023

rusher May 1, 2023

bgrainger Apr 29, 2023

bgrainger Apr 29, 2023

bgrainger Apr 29, 2023

rusher May 1, 2023

bgrainger left a comment

		ProtocolCapabilities.DeprecateEof \|

		// ProtocolCapabilities.DeprecateEof \|

Method	Mean	Error	StdDev	Allocated
Normal	3.190 ms	0.0975 ms	0.2766 ms	3.43 KB
Prepared	3.983 ms	0.0783 ms	0.1490 ms	3.08 KB

Permit real prepared stored procedure #1314

Are you sure you want to change the base?

Permit real prepared stored procedure #1314

Conversation

rusher commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrainger left a comment

Choose a reason for hiding this comment