Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Since there can be pretty major performance problems when your database isn't set to READ-COMMITTED, it's a good idea to implement a warning message for the users and guide then into a way to solve the performance problem.
Remaining tasks
Create a hook requirement to validate the isolation level inside mysql module, this hook MUST work with MySQL 5.7+- Test
- Review
- Commit
Proposed resolution
Implement a warning message inside status report page.
User interface changes
After this issue lands:
API changes
None
Data model changes
None
For the committer
Could everybody who worked on https://www.drupal.org/project/documentation/issues/3266063 as get a commit credit on this issue.
Release notes snippet
Drupal core will begin warning in the status report if a mysql database connection is not using a <strong>READ-COMMITTED</strong> isolation level of transaction.
Comment | File | Size | Author |
---|---|---|---|
#65 | interdiff_2733675_61-65.txt | 1.99 KB | andregp |
#65 | 2733675-65.patch | 7.35 KB | andregp |
#61 | interdiff_2733675_58-61.txt | 1.33 KB | andregp |
#61 | 2733675-61.patch | 7.34 KB | andregp |
Comments
Comment #2
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedPatch attached, basically copied what is done in apdqc, we just don't need all the other warnings it has to warrant adding the module as a dependency.
Comment #3
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedCleaned up the wording and display a bit
Comment #4
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedScreenshot of message
Comment #5
rszrama CreditAttribution: rszrama at Centarro commentedWouldn't really support a change like this as it's quite aggressive for most users. There's certainly a question of how to make this setting more known to users who are experiencing the issue, but the most I'd do through hook_requirements() is make this an info line as opposed to even a warning (or certainly an error).
I think it might be wise to link such a line to a documentation page on drupal.org that we can keep up to date with information on how to address the issue instead of trying to include a suggestion in the status report itself. Easier to keep up to date, link to external resources, offer multiple strategies, etc.
Comment #6
mglamanI agree, we should have it a warning, not error. And write up a good documentation node on D.o. I say warning because what if it's. A smaller site and their hosting doesn't allow them to change it, or something.
Comment #7
mglamanPhone hijacked status
Comment #8
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedChanged down to info, wasn't really thinking about smaller sites, but you're right, it may be totally irrelevant for some sites and a constant error or warning is overkill.
As for documentation, I was thinking I could add a performance page to the end of the documentation, I can add this to start and build on a few more things. Is this https://www.drupal.org/node/1007414 the preferred place for 7.x documentation still?
Comment #9
mglamanYep, that node is the main place for 1.x
Comment #10
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedAight, documentation page made as well, only has locking so far, and I just copied the stuff from my blog post, but I think it's good.
https://www.drupal.org/node/2734489
Also attached new screenshot
Comment #11
mglamanLooks good when I ran it through Simplytest.me
However, we should probably link to the documentation from here, too.
Comment #12
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedDescription cleaned up a little and link to documentation added
Comment #13
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedRTBC+1...Works like a charm. Fyi warning disappears when the setting is added to settings.php. I assume this is intentional.
Comment #14
jonathanshawShould this issue be moved to D8?
Comment #15
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedI wonder if it should even be moved to Core instead of commerce, as it turns out this happens a lot for non-commerce sites as well.
Comment #16
heddnI think this would get greater tracking in the new core status page. Shall we move queues?
Comment #17
jonathanshawLet's see if anyone objects to that.
Comment #18
jonathanshawNot sure of the component
Comment #19
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedRerolled for Drupal 8 core
Comment #20
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedUpdated to check only when using MySQL and use session and not global for the suggested settings.
Comment #21
jonathanshawHmm, the wider Drupal context is complicated.
Comment #22
Darren OhComment #29
alexpottDiscussed with @catch and we agreed that as a first step to promote this to critical instead of #1650930: Use READ COMMITTED by default for MySQL transactions because it is easier to implement and will benefit all users right now.
The code needs to be updated to work with MySQL 8 as well. Plus this can now go in the mysql module instead of system requirements.
Also in the description we could link to a page on d.o or give the information how to fix it within your settings.php. You can add an
init_commands
section to your db connection settings and do:Comment #30
catchComment #31
froboyFor anyone who's working on making this change, I was trying to test to ensure that the command was being applied correctly, but unfortunately drush doesn't apply these overrides so testing with
drush sqlq "SELECT @@tx_isolation;"
never reflected the override.Instead I was able to use a variation on the code in the above patch, modified because... I couldn't get drush to handle the nested quotes well.
drush ev 'dump(\Drupal::database()->query("SELECT @@tx_isolation;")->fetchAssoc());'
should print the value of the variable to confirm that the change is being applied, like:Comment #32
geek-merlin@froboy: Thanks, that's really helpful and surely saved me from some WTF confusion.
Comment #33
murilohp CreditAttribution: murilohp at CI&T commentedI've added a new requirement hook inside mysql module for the transaction isolation level.
Regarding the test part, I'm facing some issues here, my idea was to create a functional test to validate the warning message on the status page report before and after changing the isolation level, but for some reason the tests are not passing here. That's why the patch has only the hook part.
FYI: The test I was writing and couldn't make it work is the following one:
Comment #34
alexpott@murilohp you're going to need to set the transaction level in settings.php in the test. See \Drupal\Tests\mysql\Functional\Mysql8RequirePrimaryKeyUpdateTest::prepareSettings() for an example of how to do this.
Comment #35
murilohp CreditAttribution: murilohp at CI&T commentedThanks for the help @alexpott, I tried my best here, unfortunately I had to split the test into two files, I had no idea about how to change the settings inside one test only. So the first test validates the warning message being displayed and the second one checks for warning message when the init command is set.
Comment #36
murilohp CreditAttribution: murilohp at CI&T commentedYou're probably wondering why I haven't used the
$modules
variable inside the test class, well I tried, but for some reason the 'mysql' module wasn't being installed, the test was failing and installing by using the 'module_installer' service solved the problem.Comment #37
murilohp CreditAttribution: murilohp at CI&T commentedComment #38
murilohp CreditAttribution: murilohp at CI&T commentedComment #39
murilohp CreditAttribution: murilohp at CI&T commentedComment #40
alexpottWe don't need an UpdatePathTestBase test - there's no update to test. Sorry for confusing you by pointing to an update path test... it's just the prepare settings stuff you need.
This can go in the non-upgrade test and it'll work just fine. In fact you can do
In your test code and set the transaction isolation level to different states in the test to test both sides. One thing that is interesting is that DrupalCI's mysql is not using READ COMMITTED - I wonder what it is using.
Plus we need to run the test on MySQL 8 and Maria to test is doesn't fail on those environments.
Comment #41
murilohp CreditAttribution: murilohp at CI&T commentedThanks @alexpott, I've updated the test and now I was able to test both scenarios.
Regarding #40:
Good catch, thinking about that, during the test I change the isolation level twice, this way I think we guarantee the level and don't have to worry about the CI configuration.
Comment #42
murilohp CreditAttribution: murilohp at CI&T commentedFixing the cspell problem
Comment #43
murilohp CreditAttribution: murilohp at CI&T commentedComment #44
murilohp CreditAttribution: murilohp at CI&T commentedComment #45
joachim CreditAttribution: joachim as a volunteer commented', [
This line's scary.
Is Drupal ALWAYS at risk of deadlock issue for fields? Why? Is there documentation on this? Is there an issue to fix it?
Comment #46
jonathanshawDrupal can suffer from deadlock issues related to fields. For the best database performance, set the transaction isolation level to READ-COMMITTED. For more information see the <a href=":performance_doc">documentation page</a> on drupal.org. To change this setting, add this to your settings.php below the $databases array <p><code>@line
1. The linked page https://www.drupal.org/node/2734489 is not from core and should not be given here.
2. I don't think we should be giving a code fix in a requirements message.
3. Maybe we should add the fix to settings.php, but commented out like many other settings are.
4. It would be good to add a documentation page at https://www.drupal.org/docs/8/system-requirements
I suggest
A. Create a new documentation page for this?
B.
For the best performance and to minimize locking issues, the READ-COMMITTED transaction isolation level is <a href=":performance_doc">recommended</a>.
Comment #47
murilohp CreditAttribution: murilohp at CI&T commentedThanks for the reviews!
#45:
This issue is a followup from #1650930: Use READ COMMITTED by default for MySQL transactions, if you check the issue, you're gonna see a lot of users facing this problem, and the proposed solution was to change the default isolation level, but before doing that, we need to warn the users(see #29), about the documentation, @daffie created a new issue to provide this doc #3266063: New page for database transaction isolation setting for MySQL, now I think that maybe review the doc before this is the best approach here.
I hope I've answered your questions @joachim, unfortunately I don't know the exact reason for the deadlocks :(
#46:
Comment #48
jonathanshawRe 47.3. As #1650930: Use READ COMMITTED by default for MySQL transactions is postponed on this issue, it doesn't address the point. I think in this issue we should add a version of the comment below to default.settings.php.
Comment #49
murilohp CreditAttribution: murilohp at CI&T commentedMy bad @jonathanshaw, when I said "I think this was implemented at the parent issue", I was thinking that, after this issue lands, the documentation could be handled on the parent issue, but I agree with you, if we could add the docs right now, it would be better. So this new patch is adding the docs on default.settings.php, and following your suggest from #46.B. This still needs work because we have to change the documentation link, thank you for helping me!
Comment #50
jonathanshawGreat.
The text you've used is copied from #1650930: Use READ COMMITTED by default for MySQL transactions. The version I gave in #48 was edited for more grammatical english and to fit the context of this issue better, I suggest using it instead.
Nit: missing period at end of sentence.
Comment #51
Johnny Santos CreditAttribution: Johnny Santos at CI&T commentedI will be working on this ( commentary #50)
Comment #52
Johnny Santos CreditAttribution: Johnny Santos at CI&T commentedJust changed the content as described in #48 and #50.
Comment #53
jonathanshawNit: Don't we usually almost always use protected not private? Not sure if this matters in tests or not.
I think this is RTBC once the blocking docs page is sorted.
Comment #54
murilohp CreditAttribution: murilohp at CI&T commentedI think it's does not matter at all but we can change this after getting the documentation done.
Moving to POSTPONED due to #3266063: New page for database transaction isolation setting for MySQL.
Comment #56
daffie CreditAttribution: daffie commented#3266063: New page for database transaction isolation setting for MySQL has been fixed. The documentation page can be found at: https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa...
Comment #57
daffie CreditAttribution: daffie commentedYou write "committed" with double t, not a single t.
This should be:
Can we change this to:
The text is then the same as on its documentation page: https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa...
This is now part of the mysql module, therefor the part with driver() == 'mysql' can be removed.
The link to the documentation page is: https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa...
Can we copy this test to after the previous page call and assert that the text exists.
Nitpick: Can we change "tx_level" to "isolation". It is what we want to use in the followup issue.
Comment #58
ankithashettyAddressed the above comments as it is, except for #57.1, where I fixed the comment spelling and removed the cspell line.
Pending: #57.6 (Din't quite understand) . Retaining status as 'Needs work' to handle this.
Thanks!
Comment #59
daffie CreditAttribution: daffie commentedThe changes to core/assets/scaffold/files/default.settings.php should be the same as those in sites/default/default.settings.php. They are not that is why the testbot is failing. The changes to core/assets/scaffold/files/default.settings.php are the correct ones.
Comment #60
andregp CreditAttribution: andregp at CI&T commented@daffie, I'll check this...
Comment #61
andregp CreditAttribution: andregp at CI&T commentedComment #62
andregp CreditAttribution: andregp at CI&T commentedI'm sorry, I chose the wrong status
Comment #63
daffie CreditAttribution: daffie commentedAll the code changes look good to me.
There is testing for the added status page warning.
I have updated the CR to be the same as the page https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa....
For me it is RTBC.
Thanks to everybody for working on this issue!
Comment #64
catchCouple of nits but otherwise looks great.
Nit, should be 'two' instead of '2'.
Should be 'Writes the isolation level in settings.php".
Comment #65
andregp CreditAttribution: andregp at CI&T commentedAddressed the points at #64.
Comment #66
daffie CreditAttribution: daffie commentedThe remarks of @catch have been addressed.
Back to RTBC.
Comment #71
catchCommitted/pushed to 10.0.x and 9.5.x, thanks!
Comment #72
alexpottI think that there are a couple of issues with this patch:
Opened #3291949: Improve the MySQL transaction isolation warning to address this.
Comment #74
daffie CreditAttribution: daffie commentedI have published the CR and I have updated the documentation page to reflect the changes in the CR.