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

SET TRANSACTION ISOLATION behaviour when using ProxySQL #1043

Closed
NataliyaLev opened this issue Mar 16, 2020 · 12 comments
Closed

SET TRANSACTION ISOLATION behaviour when using ProxySQL #1043

NataliyaLev opened this issue Mar 16, 2020 · 12 comments

Comments

@NataliyaLev
Copy link

NataliyaLev commented Mar 16, 2020

Steps to reproduce

Connect your application to MySQL via ProxySQL
Use transactions (context.Database.AutoTransactionsEnabled = true;)
You will see errors of this type:
MySQL_Session.cpp:5308:handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo(): [WARNING] Unable to parse multi-statements command with SET statement: setting lock hostgroup . Command: set transaction isolation level repeatable read; start transaction;

The issue

The issue is that:

  1. Pompelo driver issues a "SET TRANSACTION ISOLATION LEVEL REPEATABLE READ" without using SESSION keyword. ProxySQL requires SESSION keyword to avoid a situation where an application accidentally sets it for everyone.
  2. The driver issues this every time; REPEATABLE READ is already a default level in MySQL. So, there is no need to do that.

The desired behaviour:

  1. Add SESSION keyword: "SET SESSION TRANSACTION ISOLATION LEVEL ..."
  2. See if it makes sense to make the call only if the value is not the default REPEATABLE READ (or if the value is different from the global transaction isolation level @@GLOBAL.transaction_isolation).
Exception message:
"MySQL_Session.cpp:5308:handler___status_WAITING_CLIENT_DATA___STATE_SLEEP___MYSQL_COM_QUERY_qpo(): [WARNING] Unable to parse multi-statements command with SET statement: setting lock hostgroup . Command: set transaction isolation level repeatable read; start transaction;"
Stack trace:

Further technical details

MySQL version: 5.7
ProxySQL: 2.7
Operating system:
Pomelo.EntityFrameworkCore.MySql version: 2.1
Microsoft.AspNetCore.App version:

Other details about my project setup:

@mguinness
Copy link
Collaborator

This issue may belong to the underlying MySqlConnector library. Can you provide a full working example?

@NataliyaLev
Copy link
Author

NataliyaLev commented Mar 16, 2020

What do you mean by a 'full working example'?
Our dev team traced it to this portion of the EF code:

private async ValueTask<MySqlTransaction> BeginDbTransactionAsync(IsolationLevel isolationLevel, IOBehavior ioBehavior, CancellationToken cancellationToken)
{
    if (State != ConnectionState.Open)
        throw new InvalidOperationException("Connection is not open.");
    if (CurrentTransaction is object)
        throw new InvalidOperationException("Transactions may not be nested.");
#if !NETSTANDARD1_3
    if (m_enlistedTransaction is object)
        throw new InvalidOperationException("Cannot begin a transaction when already enlisted in a transaction.");
#endif
    string isolationLevelValue = isolationLevel;

    switch (isolationLevel) {

        IsolationLevel.ReadUncommitted => "read uncommitted",
        IsolationLevel.ReadCommitted => "read committed",
        IsolationLevel.RepeatableRead => "repeatable read",
        IsolationLevel.Serializable => "serializable",
        // "In terms of the SQL:1992 transaction isolation levels, the default InnoDB level is REPEATABLE READ." - http://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-model.html
        IsolationLevel.Unspecified => "repeatable read",
        _ => throw new NotSupportedException("IsolationLevel.{0} is not supported.".FormatInvariant(isolationLevel))
    
    }
    using (var cmd = new MySqlCommand("set transaction isolation level " + isolationLevelValue + "; start transaction;", this))
        await cmd.ExecuteNonQueryAsync(ioBehavior, cancellationToken).ConfigureAwait(false);
    var transaction = new MySqlTransaction(this, isolationLevel);
    CurrentTransaction = transaction;
    return transaction;
}

@mguinness
Copy link
Collaborator

mguinness commented Mar 16, 2020

@bgrainger Do you have any insight into this issue?

@bgrainger
Copy link
Collaborator

Seems like a duplicate of mysql-net/MySqlConnector#774 (originally filed as mysql-net/MySqlConnector#772).

(Ultimately it feels like a bug in ProxySQL; why can't it "parse multi-statements command with SET statement"?)

Pompelo [sic] driver issues a "SET TRANSACTION ISOLATION LEVEL REPEATABLE READ" without using SESSION keyword. ProxySQL requires SESSION keyword to avoid a situation where an application accidentally sets it for everyone.

I don't know about ProxySQL, but this isn't how MySQL Server works.

https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html#set-transaction-scope

Without any SESSION or GLOBAL keyword:

  • The statement applies only to the next single transaction performed within the session.

This is the desired behaviour, which is why MySqlConnector issues that statement.

The driver issues this every time; REPEATABLE READ is already a default level in MySQL. So, there is no need to do that.

It's "a" default level, but there's absolutely no guarantee that the server hasn't been configured with a different default level. So it should be issued every time so that ADO.NET semantics are followed, regardless of server defaults.

@bgrainger
Copy link
Collaborator

Per sysown/proxysql#1728 (comment), it appears that ProxySQL may clear the CLIENT_MULTI_STATEMENTS flag, indicating that it doesn't support multiple statements in a single batch. MySqlConnector doesn't respect this; if it did, that might work around the issue.

Secondly, per sysown/proxysql#2305 (comment)

We currently do not support SET TRANSACTION ISOLATION LEVEL without SESSION because it requires a lot of efforts

That would be harder to work around, since changing the isolation level for the connection (i.e., session) for an individual MySqlTransaction seems quite wrong.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Mar 17, 2020

@bgrainger In case MySqlConnector does support non CLIENT_MULTI_STATEMENTS connections in the future, a general approach to the SET TRANSACTION ISOLATION LEVEL issue could be to introduce a hook (or some factory to override) for people to review/change MySqlCommands before they get executed. Could be helpful for other scenarios as well.

@NataliyaLev Currently, this should be possible using a library like Harmony or of course by compiling and shipping your own version of MySqlConnector.

@NataliyaLev
Copy link
Author

This is the workaround we found for now:

{
rule_id=3
active=1
match_pattern="SET TRANSACTION ISOLATION(.*)"
replace_pattern="SELECT 1 LIMIT 0"
apply=1
}

@bgrainger
Copy link
Collaborator

it appears that ProxySQL may clear the CLIENT_MULTI_STATEMENTS flag

My interpretation of that was incorrect. The comment was indicating that that was a configuration change you could make to ProxySQL (it's not true out-of-the-box).

Moreover, the comment seems wrong; according to https://github.com/sysown/proxysql/wiki/Global-variables#mysql-client_multi_statements it takes effect "when connecting to MySQL backends". Local testing indicates that ProxySQL still presents the CLIENT_MULTI_STATEMENTS flag to MySqlConnector when it connects. Thus, we cannot use this to determine whether multiple statements should be sent.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Apr 17, 2020

@bgrainger How should we proceed here? Do you want to track this issue further on the MySqlConnector repo?

@bgrainger
Copy link
Collaborator

Yes, I think this is a duplicate of mysql-net/MySqlConnector#774.

@lauxjpn
Copy link
Collaborator

lauxjpn commented Apr 17, 2020

Okay, I close this issue here then.

@bgrainger
Copy link
Collaborator

Both these issues should be fixed in MySqlConnector 0.64.0.

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

No branches or pull requests

4 participants