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

Send single payload in BeginTransactionAsync #1286

Closed
DomoYao opened this issue Feb 24, 2023 · 6 comments
Closed

Send single payload in BeginTransactionAsync #1286

DomoYao opened this issue Feb 24, 2023 · 6 comments

Comments

@DomoYao
Copy link

DomoYao commented Feb 24, 2023

Discussion summary: Revert change in #774 and use Pipelining = false for databases that don't support a single payload. This will reduce the round-trips from 2 to 1 when starting a transaction.


private async ValueTask<MySqlTransaction> **BeginTransactionAsync**(IsolationLevel isolationLevel, bool? isReadOnly, IOBehavior ioBehavior, CancellationToken cancellationToken)
	{
		using (var cmd = new MySqlCommand($"set session transaction isolation level {isolationLevelValue};", this) { NoActivity = true })
		{
			await cmd.ExecuteNonQueryAsync(ioBehavior, cancellationToken).ConfigureAwait(false);

			var consistentSnapshotText = isolationLevel == IsolationLevel.Snapshot ? " with consistent snapshot" : "";
			var readOnlyText = isReadOnly switch
			{
				true => " read only",
				false => " read write",
				null => "",
			};
			var separatorText = (consistentSnapshotText.Length == 0 || readOnlyText.Length == 0) ? "" : ",";
			cmd.CommandText = $"start transaction{consistentSnapshotText}{separatorText}{readOnlyText};";
			await cmd.ExecuteNonQueryAsync(ioBehavior, cancellationToken).ConfigureAwait(false);
		}

		var transaction = new MySqlTransaction(this, isolationLevel);
		CurrentTransaction = transaction;
		return transaction;
	}

Can't the default level of the database be used?
This class seems to be one more database operation.

@bgrainger
Copy link
Member

What do you propose instead?

@bgrainger
Copy link
Member

@DomoYao
Copy link
Author

DomoYao commented Feb 27, 2023

What do you propose instead?
Thanks @bgrainger .
I think, no transaction level specified, no transaction setup.

if (isolationLevel != IsolationLevel.Unspecified)
{
await cmd.ExecuteNonQueryAsync(ioBehavior, cancellationToken).ConfigureAwait(false);
}

I tested that one less "set session transaction isolation level repeatable read;" script execution.There is a performance gain, but I don't know if there are other effects

@bgrainger
Copy link
Member

This would be a significant behavioural change for all existing users if connection.BeginTransaction() no longer set a default isolation level and used the database default instead.

If we need to optimise performance, I'd rather revert #774 and instruct the (small number of) users who are affected to opt out by using Pipelining = false. (It probably would have been done that way in the first place if the Pipelining connection string option existed when that issue was fixed.)

Is there any other reason to make this change, other than reducing DB roundtrips?

@DomoYao
Copy link
Author

DomoYao commented Feb 28, 2023

Thank @bgrainger , my purpose is to optimise performance, through configuration optimization is a better solution.

@bgrainger bgrainger changed the title Why do you need to set the transaction level every time you start a transaction? Send single payload in BeginTransactionAsync Nov 12, 2023
@bgrainger
Copy link
Member

Fixed in 2.3.6.

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

No branches or pull requests

2 participants