PomeloFoundation / Pomelo.EntityFrameworkCore.MySql

Entity Framework Core provider for MySQL and MariaDB built on top of MySqlConnector
MIT License
2.7k stars 383 forks source link

Changes not committed by SaveChanges for single-statement updates (probably EF7-related) #1740

Open nightcoder62 opened 1 year ago

nightcoder62 commented 1 year ago

Summary

After upgrading a project to EF7 and Pomelo.EntityFrameworkCore.MySql 7.0.0-silver.1, certain changes to the database stopped working, without any errors. Everything seemed to work, but the changes were never committed to the database. As far as we can see, this affected only changes that generated single statements (as in deleting a record from a table).

The details haven't been fully investigated, but we finally found a workaround, and I'm creating this issue in case others encounter the same problem - there's also a chance that the cause is obvious to the maintainers when they see this. I'll try to do some further testing and research over the weekend.

Steps to reproduce

In a .NET 7.0 (Windows x64) project, we use these nuget packages (and whatever else is added automagically):

We connect to a database that runs MySQL version 5.6.21-log, using InnoDB. Yes, it's old but that's out of our control at the moment.

Configuration of our DbContext:

            services.AddDbContext<TestDbContext>(option =>
            {
                option.UseMySql(testDbConnectionString, new MySqlServerVersion("5.6.21-log"))
                    .EnableSensitiveDataLogging()
                    .EnableDetailedErrors();
            });

The issue

Single-statement database changes (SaveChanges) seem to work according to all logging, but the changes never get committed.

Running this on a freshly injected context results in one affected record, but it's still in the database (note that CurrentTransaction on context.Database is null, as expected, before the second line is executed):

context.Remove<SomeEntity>(entity);
var count = context.SaveChanges(); // count is 1 after execution

No exceptions, no error messages. The log looks like this:

Microsoft.EntityFrameworkCore.Database.Command: Information: Executed DbCommand (17ms) [Parameters=[@p0='1234'], CommandType='Text', CommandTimeout='30']
DELETE FROM `SomeTable`
WHERE `Id` = @p0;
SELECT ROW_COUNT();

Workaround

Add this in the constructor for the context:

Database.AutoTransactionBehavior = AutoTransactionBehavior.Always;

This makes the problem go away.

Suspicion

When AutoTransactionBehaviour is the default (AutoTransactionBehaviour.WhenNeeded), autocommit (MySql "SET autocommit=0") is set to 0 somewhere, which is wrong (it doesn't turn off implicit transactions, it just turns off committing them automatically). InnoDB starts implicit transactions ALWAYS, and it can't be turned off - a commit is ALWAYS required, disabling autocommit just stops it from happening automatically.

Further details

See my comments on #1620 - as you can see, I got some help from @roji on this.

This is the change in EF7 that this bug is most likely caused by: https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-7.0/whatsnew#unneeded-transactions-are-eliminated

lauxjpn commented 1 year ago

The Pomelo provider currently does not issue any autocommit statements (we might change that in the future, in a similar way as SQL Server does). Since according to the MySQL docs, autocommit always starts as enabled when a session is created, it is likely that the app's code, the connection string or the database configuration disables it:

15.7.2.2 autocommit, Commit, and Rollback

In InnoDB, all user activity occurs inside a transaction. If autocommit mode is enabled, each SQL statement forms a single transaction on its own. By default, MySQL starts the session for each new connection with autocommit enabled, so MySQL does a commit after each SQL statement if that statement did not return an error. [...]


@nightcoder62 To get to the bottom of this issue, let's start by checking first, that autocommit is actually turned off right before the entity is supposed to be removed (so right before the SaveChanges() call), by executing the following statement from the MySQL connection your DbContext is using from within your app:

select @@global.autocommit, @@session.autocommit;

For example:

context.Database.OpenConnection();
var connection = context.Database.GetDbConnection();

using var command = connection.CreateCommand();
command.CommandText = "select @@global.autocommit, @@session.autocommit;";

using var dataReader = command.ExecuteReader();
if (dataReader.Read())
{
    var globalAutoCommit = dataReader[0];
    var sessionAutoCommit = dataReader[1];

    Logger.LogInformation($"@@global.autocommit = {globalAutoCommit}, @@session.autocommit = {sessionAutoCommit}");
}

Then check the log, whether it is the global or session autocommit, that is 0. Once we know that, we can continue investigating the issue further.

nightcoder62 commented 1 year ago

@lauxjpn yes, I mentioned such suspicions (configuration or app issue - it's a big app) in one of the linked threads, so reproducing the problem in a cleanroom setting is next on my list.

I'm a bit pressed for time though, but using your suggestion to check the current value of autocommit is quicker. I'll try to find some time to do that. Thanks!

nightcoder62 commented 1 year ago

Spot on, @lauxjpn , ping @roji - your (and my) suspicion was true. Someone, somewhere, years ago, has turned autocommit off globally server-side. I may not ever find the reason, and I'm not the server admin so it'll take some time until I can get it fixed - but since the workaround (reverting EF7 to EF6 behaviour) makes the problem go away, we're fine. Most likely, we won't even change the setting - there are old legacy systems working with the database too, and it could be a workaround for something.

I leave it for you to decide whether or not to close this issue, but might I suggest mentioning the workaround in a readme for the EF7 support, since everything works perfectly with global autocommit = 0 in EF6 but fails in EF7 (without the workaround)?

Fabulous adventure ... :smile:

Oh, and @roji, this (the workaround, mainly) should perhaps be mentioned and highlighted in the "what's new" docs for EF7 as well? I don't know if the cause for someone changing that setting here is a common thing, but I'm seeing old articles on SO about how to do it, so we may not be alone. It's pretty hairy, as it could result in data loss.

lauxjpn commented 1 year ago

@nightcoder62 Glad you found the issue!

Using your AutoTransactionBehavior.Always workaround seems to be the simplest solution for now.

The other way would be to use a connection interceptor (overriding ConnectionOpened() and ConnectionOpenedAsync()) to execute a set autocommit = 1; statement.


We might explicitly set autocommit = 1 before the GA release, similar to SQL Server. I will do some testing on that and will close this issue after we have made a decision.

There are a couple of assumptions that Pomelo makes about the database server configuration for all features to work as expected, which we should document.

roji commented 1 year ago

Oh, and @roji, this (the workaround, mainly) should perhaps be mentioned and highlighted in the "what's new" docs for EF7 as well?

This problem is specific to MySQL, since we disable implicit transactions for SQL Server and most other databases (PG, SQLite...) don't have such a mode. So I'm not sure it's appropriate to mention in the general EF what's new docs.

nightcoder62 commented 1 year ago

@roji wrote:

This problem is specific to MySQL, since we disable implicit transactions for SQL Server and most other databases (PG, SQLite...) don't have such a mode. So I'm not sure it's appropriate to mention in the general EF what's new docs.

I agree that this specific problem may be a bit too obscure for the docs, but a sentence that mentions the possibility to revert to the old transaction behaviour (using AutoTransactionBehaviour) would be prudent, I think. There could be similar interactions in other systems and situations. But that's just my 2 cents, I trust your team to make the right decision - you have the bigger picture.

Remember, you gave me the solution before I found it myself. OK, I had only just started looking for it, but I suspect it could have taken me some time. Had it been mentioned in the what's new docs (which I had read carefully - more than once), we might have solved the issue even quicker.

In any case, we're extremely grateful to the one of you guys who suggested that it should be possible to revert to the old behaviour in the first case - it saved our day. And the decision for now is to leave the setting as is, because we can't risk some legacy system breaking due to us changing it - that's life in the in-house trenches*. :cry:

*) The solution the problem happened in is, in fact, a bunch of apps and services that are, one by one, replacing all those legacy systems, so we're on it. But that doesn't happen overnight. :smile:

nightcoder62 commented 1 year ago

@lauxjpn I'd love that documentation. I have a pilot friend who sometimes let me ride shotgun in a Cessna or something, and in the club, there's a big poster on the door that says "Pilots". The poster reads: "The flight isn't over until the paperwork is done". The paperwork is dull, but yes, it has to be done. If you forget to close your flight plan (as in "Sweden Control, I'm home"), it can get expensive, depending on the amount of search and rescue alerted. I tend to think it's the same with code. Unless it's my code, of course ...

Nefcanto commented 1 year ago

For me adding that this.Database.AutoTransactionBehavior = AutoTransactionBehavior.Always; did not work.

Still SaveChanges does not commit. Though in the log I see this:

info: 01/24/2023 13:20:54.512 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (41ms) [Parameters=[@p0=NULL (Size = 4000), @p1='0a06f31f-9bb7-11ed-a5a2-0242ac140002', @p2=NULL (DbType = Guid), @p3=NULL (Size = 4000), @p4=NULL (DbType = Guid), @p5=NULL, @p6=NULL, @p7=NULL (DbType = Int32), @p8=NULL (DbType = Int32), @p9='0', @p10=NULL (DbType = Int64), @p11='hi' (Size = 4000)], CommandType='Text', CommandTimeout='30']
      SET AUTOCOMMIT = 1;
      INSERT INTO `Hierarchies` (`Description`, `EntityTypeGuid`, `IconGuid`, `IconSvg`, `ImageGuid`, `IsActive`, `IsLeaf`, `ItemsCount`, `Level`, `Order`, `ParentId`, `Title`)
      VALUES (@p0, @p1, @p2, @p3, @p4, @p5, @p6, @p7, @p8, @p9, @p10, @p11)
      RETURNING `Id`, `Guid`, `Key`, `Slug`;

Though it says SET AUTOCOMMIT = 1;, but it does not commit and I need to use set transaction isolation level read uncommitted; to be able to see the record in the database.

What else should we do to solve this? I'm using <PackageReference Include="Pomelo.EntityFrameworkCore.MySql" Version="7.0.0" />.

When I just call SaveChanges for example for a blog post, then it would be saved at last. I don't know when exactly, but it would be persisted.

However, in our architecture, sometimes we insert a record into a table using SaveChagnes then we immediately read it from another view using dbSet.Find(entity.Id) to get more related info. This is where it fails. When we call the Find method, the entity that is returned from SaveChanges is null.

roji commented 1 year ago

@Nefcanto a minimal code repro showing this behavior would, as always, be the best way forward.

Nefcanto commented 1 year ago

@roji, since creating a repro is time-consuming, can you please tell me when the auto-commit would do the job of committing? I mean, would it do it when the DbContext is disposed of, or does it do it in a time-based manner?

roji commented 1 year ago

Auto-commit (the default) just means that when you send a SQL statement to the database without having started an (explicit) transaction beforehand, that statement is executed and committed immediately (as if it were wrapped inside an explicit transaction containing only itself). When auto-commit is off, that generally means that sending a SQL statement starts a new transaction, which you later much explicitly commit or rollback. I'm not aware of any timer-based mechanics around this in any database.

lauxjpn commented 1 year ago

@Nefcanto We will indeed need a repro for that. You can use the Program.cs code from https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/issues/1600#issuecomment-1013953253 as a skeleton.

Also, please share with us the database version you are using (and any setup out of the ordinary).

Nefcanto commented 1 year ago

Well, after we prepared a MRE, we realized that it works just fine. As we investigated, it turned out to be a bug in our view that one colleague caused. The view did not return the record because of a wronly configured join.

However, when we put "SaveChanges" and "Find" after each other and from two different instantiated contexts, it returns null. I'm still investigating this.

saikotek commented 1 year ago

The Pomelo provider currently does not issue any autocommit statements (we might change that in the future, in a similar way as SQL Server does). Since according to the MySQL docs, autocommit always starts as enabled when a session is created, it is likely that the app's code, the connection string or the database configuration disables it:

15.7.2.2 autocommit, Commit, and Rollback

In InnoDB, all user activity occurs inside a transaction. If autocommit mode is enabled, each SQL statement forms a single transaction on its own. By default, MySQL starts the session for each new connection with autocommit enabled, so MySQL does a commit after each SQL statement if that statement did not return an error. [...]

@nightcoder62 To get to the bottom of this issue, let's start by checking first, that autocommit is actually turned off right before the entity is supposed to be removed (so right before the SaveChanges() call), by executing the following statement from the MySQL connection your DbContext is using from within your app:

select @@global.autocommit, @@session.autocommit;

For example:

context.Database.OpenConnection();
var connection = context.Database.GetDbConnection();

using var command = connection.CreateCommand();
command.CommandText = "select @@global.autocommit, @@session.autocommit;";

using var dataReader = command.ExecuteReader();
if (dataReader.Read())
{
    var globalAutoCommit = dataReader[0];
    var sessionAutoCommit = dataReader[1];

    Logger.LogInformation($"@@global.autocommit = {globalAutoCommit}, @@session.autocommit = {sessionAutoCommit}");
}

Then check the log, whether it is the global or session autocommit, that is 0. Once we know that, we can continue investigating the issue further.

so what is default behaviour of Pomelo provider when we call SaveChanges()? If I edit 2 different tables, one update fails and one succeds will it rollback all changes automatically without having to use BeginTransaction() explicitly?