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

Ensure AUTOCOMMIT is set to 1 where needed #1744

Merged
merged 1 commit into from
Jan 16, 2023

Conversation

lauxjpn
Copy link
Collaborator

@lauxjpn lauxjpn commented Jan 16, 2023

Similar to SQL Server, we force a SET AUTOCOMMIT = 1; statement before statements, that are executed without explicit transaction, just in case the server or session default was previously changed to AUTOCOMMIT = 0.

Fixes #1740

@lauxjpn lauxjpn added this to the 7.0.0 milestone Jan 16, 2023
@lauxjpn lauxjpn self-assigned this Jan 16, 2023
@lauxjpn lauxjpn merged commit 4ad5d4d into PomeloFoundation:master Jan 16, 2023
@lauxjpn lauxjpn deleted the fix/issue1740 branch January 16, 2023 18:16
@lauxjpn lauxjpn mentioned this pull request Jan 16, 2023
@nightcoder62
Copy link

nightcoder62 commented Nov 24, 2023

@lauxjpn : I was the one writing the original issue, and in the exact same system I ran into the same problem again, but this time with the new ExecuteDelete method. I think this fix is not implemented there. Wrapping the work in a transaction works well, so we have no issues at the moment, but perhaps it should be implemented for any other actions (ExecuteDelete, ExecuteUpdate, ExecuteSql and so on) that may experience the same problem with that setting?

Again - I acknowledge that it's really weird and probably extremely rare to have autocommit=0 as a default in a server, but like I wrote before, we can't change it due to some legacy database tool that is still in use here ...

EDIT (some additional thoughts):

We are still using the AutoTransactionBehavior.Always workaround (nobody got around to removing it when we upgraded your provider), but it doesn't help in this case. THAT is perhaps an EF problem? As I see it, the AutoTransactionBehavior setting should apply to the "Execute..."-methods as well - but I haven't checked if the docs say anything about that, so I may be wrong. Any thoughts on that? Also, I didn't look at your code - do you enable this fix if AutoTransactionBehaivor is AutoTransactionBehavior.Always?

Haven't tested with SQL Server with IMPLICIT_TRANSACTIONS set to ON by default (which is equivalent to autocommit=0) yet, but I think I will. It would be interesting to know if that would result in the same problem.

So, as I see it, it may be two different problems:

  1. One-liners should be committed even if the server setting is autocommit=0 and AutoTransactionBehavior is set to AutoTransactionBehavior.WhenNeeded. In this case, the provider(s) should issue the command "set autocommit=1" (or "SET IMPLICIT_TRANSACTIONS OFF" in SQL Server) before the other SQL statement(s).

  2. When AutoTransactionBehavior is set to AutoTransactionBehavior.Always, any action that runs statements (any of the Execute...-methods, NOT just the SaveChanges...-methods) should always be wrapped in a transaction. UNLESS it's documented that this won't happen automatically (has it ever?).

Your thoughts? Let me know if I'm not making sense ... it happens, at times.

@roji
Copy link

roji commented Nov 24, 2023

@nightcoder62 currently, AutoTransactionBehavior applies only to SaveChanges, and not to ExecuteUpdate and ExecuteDelete; this is by design. As another example, when a LINQ query produces multiple SQL queries (i.e. because of split query), no transaction is started regardless of AutoTransactionBehavior, although such a transaction could guarantee a consistent view across the multiple queries (depending on the isolation level).

In other words, the same behavior currently happens on the SQL Server provider as well. If you think this is wrong, you can open an issue on the EF repo and we can discuss this.

@nightcoder62
Copy link

nightcoder62 commented Nov 24, 2023

@roji I suspected that this was the case. Interesting. Again, it must definitely be an edge case to have the server setting (or the SQL Server variety of it) we have, so I'm not decidedly thinking the current EF (or provider) behaviour is wrong - I could easily accept it as being "by design" and be OK with it. In either case, thanks for confirming that it is indeed an EF "problem" (if it is a problem) and not specific to the MySQL provider.

Also, I haven't studied the docs (but I will). If I think they can be improved (and feel that it's needed), I'll open an issue there.

Thanks again!

@roji
Copy link

roji commented Nov 24, 2023

@nightcoder62 great. It would be good if you could open the issue so we can at least discuss it internally and make sure we're all aligned on this behavior.

@nightcoder62
Copy link

@roji : As it's almost midnight here in Sweden (and my nick is no longer as relevant as it was 20 years ago - especially not on a Friday night after a Scotch or two), I'll let it rest until tomorrow. And possibly think it through before I post it. But yes, there'll be an issue over on EF core in a day or two. :D

@roji
Copy link

roji commented Nov 24, 2023

Oh no urgency at all @nightcoder62 :) Just wanted to make sure this comes up in our discussions at some point, is all. Thanks!

@nightcoder62
Copy link

@roji Added an issue last week, so we'll see what happens. Thank's for being there! And that, of course, goes to @lauxjpn as well. :D

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

Successfully merging this pull request may close these issues.

Changes not committed by SaveChanges for single-statement updates (probably EF7-related)
3 participants