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

[11.x] MySQL transaction isolation level fix #50689

Merged
merged 4 commits into from
Mar 21, 2024

Conversation

mwikberg-virta
Copy link
Contributor

Should fix #50688

@mwikberg-virta mwikberg-virta changed the title Mysql transaction isolation level fix MySQL transaction isolation level fix Mar 21, 2024
@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 21, 2024

This is incorrect. It already runs SET SESSION TRANSACTION ISOLATION LEVEL %s;. Please read the surrounding code or look at your general log. Or even the tests that you edited show this running SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ, NAMES 'utf8' COLLATE 'utf8_unicode_ci';.

@GrahamCampbell
Copy link
Member

What version of mysql are you using?

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 21, 2024

Oh man, it does seem the transaction isolation level is a special beast. I'm going to propose a different fix.

@mwikberg-virta
Copy link
Contributor Author

8.3. The current code concatenates all the "SET" statement into a single statement, which does not work for the isolation level.

SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED, NAMES 'utf8mb4' COLLATE 'utf8mb4_unicode_ci', SESSION sql_mode='ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION';

This results in a SQL error:
[2024-03-20 15:50:14] [42000][1064] You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NAMES 'utf8mb4' COLLATE 'utf8mb4_unicode_ci', SESSION sql_mode='ONLY_FULL_GROUP_' at line 1

@mwikberg-virta
Copy link
Contributor Author

Setting only the NAMES and sql_mode works just fine in a single statement

@mwikberg-virta
Copy link
Contributor Author

Anyhow, any fix that works fine is appreciated. Just bumped into this while checking the requirements to upgrade a project to 11.x and this at least made the tests pass

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this looks correct, after digging around. It seems MySQL implemented transaction isolation level differently to everything else, it's the one thing I didn't manually test, and its rarely used which is why we managed to get this far with nobody noticing.

@GrahamCampbell GrahamCampbell changed the title MySQL transaction isolation level fix [211.x] MySQL transaction isolation level fix Mar 21, 2024
@GrahamCampbell GrahamCampbell changed the title [211.x] MySQL transaction isolation level fix [11.x] MySQL transaction isolation level fix Mar 21, 2024
@mwikberg-virta
Copy link
Contributor Author

Assumed that it's not a widely used scenario, and it's mostly only used while running tests in our case (where we kindof need to see the uncommitted data, as all tests run in a transaction)

@GrahamCampbell
Copy link
Member

and it's mostly only used while running tests in our case (where we kindof need to see the uncommitted data, as all tests run in a transaction)

You run multiple php processes that both need the same data? Transactions can read their own writes without changing the isolation level. Moreover, the uncommitted isolation level makes it possible for transactions to not actually be able to read their own writes in some situations.

@mwikberg-virta
Copy link
Contributor Author

Let's just say "it's complicated". There are several systems involved and some of the tests are more if the integration test kind, where a different component in a separate thread/container/whatever expects the data to be in the DB (which it will not be, as the original transaction is not committed). Then again, setting the default DB value to READ UNCOMMITTED would break other tests, which again rely in the transactional behavior being "as expected".

@driesvints driesvints requested a review from taylorotwell March 21, 2024 09:06
@taylorotwell taylorotwell merged commit 202e521 into laravel:11.x Mar 21, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using transaction isolation level breaks the whole database connection
3 participants