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

Transaction isolation level should not default to REPEATABLE READ regardless of server configuration if left unspecified #1442

Open
bdach opened this issue Jan 26, 2024 · 3 comments

Comments

@bdach
Copy link

bdach commented Jan 26, 2024

Software versions
MySqlConnector version: 2.1.6, but still relevant on latest master as far as I can tell
Server type (MySQL, MariaDB, Aurora, etc.) and version: Not relevant
.NET version: Not relevant

Describe the bug
If a transaction is started without specifying an isolation level, it will silently default to IsolationLevel.Unspecified, which is really just REPEATABLE READ:

// "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",

The comment is correct in stating that REPEATABLE READ is the default innodb transaction isolation level, however, this is confusing in cases where the global transaction isolation level on the mysql instance that queries are directed to is deliberately changed from the configuration default.

Related: #772

Exception
Not applicable

Code sample
See usage. On a mysql transaction with READ COMMITTED default transaction isolation level this would instead force REPEATABLE READ and as such caused a long investigation into ensuing deadlocks, that was "fixed" by ppy/osu-queue-score-statistics@5c23e0f.

Expected behavior
MySqlConnector does not attempt to explicitly set any transaction isolation level when starting a transaction with IsolationLevel.Unspecified.

Additional context
Not applicable

@bgrainger
Copy link
Member

I think this comment explains the current behaviour: PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#1043 (comment)

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.

And https://learn.microsoft.com/en-us/dotnet/api/system.data.common.dbconnection.begintransaction?view=net-8.0#remarks:

If you do not specify an isolation level, the default isolation level for the specific type of connection is used.

IMO, the documentation doesn't state or imply that an implicit server-set default will be used if not specified by the client.

I'm a little reticent to change this because

  • it's been this way for a very long time (since 783645f) and would be a silent breaking change for almost all users of the library
  • it has a very simple workaround: explicitly specify the isolation level you want

@bdach
Copy link
Author

bdach commented Jan 26, 2024

I can understand that this would be a pretty big API break and the use case is likely niche, but I hope it is also understandable that the current behaviour results in a rather unexpected and confusing outcome. Maybe at least a remark in documentation could be added for posterity?

@bgrainger
Copy link
Member

Yes, this should certainly be clearly documented.

FWIW, MySql.Data also defaults to REPEATABLE READ: https://github.com/mysql/mysql-connector-net/blob/0bb0e8c4d9f327138e758b6a31c3d8ebb70a2eae/MySQL.Data/src/MySqlConnection.cs#L490-L492

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