Use READ COMMITTED by default for MySQL transactions

Fixed the Drupal\Tests\mysql\Functional\RequirementsTest.

Added a test to check if all tables have a primary key (if not show a warning on the status report page)

Added the patch and interdiff files (didn't add it before)

Fixed code standards

Regenerated phpstan baseline

1. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -175,6 +175,18 @@ public function getFormOptions(array $database) {
   +        'READ COMMITTED' => t('Read Committed'),
   +        'REPEATABLE READ' => t('Repeatable Read'),
   +        'DO NOT SET BY DRUPAL' => t('Do not set by Drupal'),
   

   $this->t() is available and preferred to t()

2. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -175,6 +175,18 @@ public function getFormOptions(array $database) {
   +      '#description' => t('The recommended transaction isolation level is "READ COMMITTED". The "Do not set by Drupal" option will not get this setting as do existing sites. Sites without this setting will default to the database set transaction isolation level. Which when not set defaults to "REPEATABLE READ".'),
   

   I think we need to have a link to something that explains what this is and the text needs finessing to be more user friendly. I also think the text "Do not set by Drupal" needs works to. It's always better to have a positive than a negative. So something "Leave to database server configuration" would be better.

3. +++ b/core/modules/mysql/mysql.install
   @@ -41,6 +41,31 @@ function mysql_requirements($phase) {
   +        'value' => (!$missing_primary_key) ? "Primary keys exist" : "Primary keys missing",
   

   Need to use translatable strings here. I'm not convinced this requirement should be shown if there are no missing primary keys. But also should we be doing this check if the isolation level is set to REPEATABLE-READ?

   Also if you do have tables without a primary key what can you do? If they are your custom modules then fine but if they are a contrib module or core you going to need to be told to create an issue.

Also if you do have tables without a primary key what can you do? If they are your custom modules then fine but if they are a contrib module or core you going to need to be told to create an issue.

Yes, excellent point, I think we need to provide a way for the site owner to action this.

Addressed the comment #219 as commented some changes required, so upload patch against #218 and inter_diff file along with it. Please review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Reroll of #221, there was a slight fuzz on the phpstan baseline changes.

I think we are still missing all changes in #219.2 and 219.3

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Lets try that again....

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Huh?! These patches are being rolled directly from 10.1.x-dev. I don't know how they keep failing to apply.

Rejected hooks, FYI

diff a/core/phpstan-baseline.neon b/core/phpstan-baseline.neon  (rejected hunks)
@@ -870,6 +870,11 @@ parameters:
                        count: 1
                        path: modules/block_content/tests/src/Functional/BlockContentTypeTest.php
 
+               -
+                       message: "#^Variable \\$goto in isset\\(\\) always exists and is not nullable\\.$#"
+                       count: 1
+                       path: modules/block_content/tests/src/Functional/PageEditTest.php
+
                -
                        message: "#^Call to deprecated constant REQUEST_TIME\\: Deprecated in drupal\\:8\\.3\\.0 and is removed from drupal\\:11\\.0\\.0\\. Use \\\\Drupal\\:\\:time\\(\\)\\-\\>getRequestTime\\(\\); $#"
                        count: 2
@@ -2175,11 +2180,6 @@ parameters:
                        count: 2
                        path: modules/system/tests/modules/entity_test/entity_test.install
 
-               -
-                       message: "#^Variable \\$goto in isset\\(\\) always exists and is not nullable\\.$#"
-                       count: 1
-                       path: modules/system/tests/src/Functional/Menu/AssertBreadcrumbTrait.php
-
                -
                        message: "#^Call to deprecated constant REQUEST_TIME\\: Deprecated in drupal\\:8\\.3\\.0 and is removed from drupal\\:11\\.0\\.0\\. Use \\\\Drupal\\:\\:time\\(\\)\\-\\>getRequestTime\\(\\); $#"
                        count: 1
@@ -3005,11 +3005,6 @@ parameters:
                        count: 12
                        path: tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
 
-               -
-                       message: "#^Variable \\$expected_driver might not be defined\\.$#"
-                       count: 2
-                       path: tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php
-
                -
                        message: "#^Relying on entity queries to check access by default is deprecated in drupal\\:9\\.2\\.0 and an error will be thrown from drupal\\:10\\.0\\.0\\. Call \\\\Drupal\\\\Core\\\\Entity\\\\Query\\\\QueryInterface\\:\\:accessCheck\\(\\) with TRUE or FALSE to specify whether access should be checked\\.$#"
                        count: 2

Rerolled from #223

Hmm, very weird. It looks like my local clone was stuck on a commit from Feb 7. Even updating the remote didn't work. Cloning fresh has fixed it, however #229 still doesn't apply

> Huh?! These patches are being rolled directly from 10.1.x-dev. I don't know how they keep failing to apply.

Did you work on the right branch? The branch name is 10.1.x, not 10.1.x-dev

Patch #229 no longer cleanly applies to D10.

Fixed patch where custom commands where failing

Added different warnings for different isolation levels and when primary keys are missing it shows the corresponding tables that do not have a primary key.

Updated the testIsolationLevelWarningNotDisplaying test to check if the proper warnings/errors are displaying on the status report page.

Custom commands failure fix

1. +++ b/core/modules/mysql/mysql.install
   @@ -31,13 +31,44 @@ function mysql_requirements($phase) {
   +      $missing_primary_key = FALSE;
   ...
   +          $missing_primary_key = TRUE;
   

   This variable is not necessary. We can also use empty($tables_missing_primary_key).

2. +++ b/core/modules/mysql/mysql.install
   @@ -31,13 +31,44 @@ function mysql_requirements($phase) {
   +      $tables_missing_key = [];
   

   Can we rename this variable to $tables_missing_primary_key

3. +++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
   @@ -203,6 +203,15 @@ public static function open(array &$connection_options = []) {
   +    if (!empty($connection_options['isolation_level']) &&
   +      in_array($connection_options['isolation_level'],
   +        ['READ COMMITTED', 'REPEATABLE READ'], TRUE)) {
   

   Can we put these 3 lines on a single line. Can we test $connection_options['isolation_level'] with strtoupper(). Then when a user sets the variable with "read committed" it also works.

4. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -81,7 +119,7 @@ public function testIsolationLevelWarningNotDisplaying() {
   diff --git a/core/modules/mysql/tests/src/Functional/mysql/PrimaryKeyExistsTest.php b/core/modules/mysql/tests/src/Functional/mysql/PrimaryKeyExistsTest.php
   

   This test is no longer necessary.

5. +++ b/core/phpstan-baseline.neon
   @@ -2955,11 +2955,6 @@ parameters:
   -		-
   -			message: "#^Variable \\$expected_driver might not be defined\\.$#"
   -			count: 2
   -			path: tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php
   -
   
   +++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php
   @@ -31,6 +31,7 @@ protected function setUp(): void {
   +    $expected_driver = '';
   

   This change is a good one, only out of scope for this issue.

6. +++ b/core/phpstan-baseline.neon
   @@ -2955,11 +2955,6 @@ parameters:
   diff --git a/core/tests/Drupal/FunctionalTests/Core/Database/DriverSpecificBrowserTestBase.php b/core/tests/Drupal/FunctionalTests/Core/Database/DriverSpecificBrowserTestBase.php
   
   +++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php
   @@ -31,6 +31,7 @@ protected function setUp(): void {
   diff --git a/core/tests/Drupal/Tests/DriverSpecificTrait.php b/core/tests/Drupal/Tests/DriverSpecificTrait.php
   

   This change is also out of scope for this issue.

made changes suggested by @daffie

Fix custom commands failing

moved $isolation_level line up

@harivansh you seem to have made changes that will break the code as you're trying an array to string conversion. Furthermore the if statement checks if the missing primary key array is not empty to determine wether you are missing primary keys. Please take a better look at the issue next time before making changes and test them accordingly.

The last submitted patch, 242: 1650930-240.patch, failed testing. View results →

All code changes look good to me.
Testing has been added.
The IS and the CR are in order.
For me it is RTBC.

1. +++ b/core/modules/mysql/mysql.install
   @@ -33,11 +33,41 @@ function mysql_requirements($phase) {
   +      $description = 'This site is using the database transaction level "READ COMMITTED".';
   ...
   +        $description = 'This site is using the database transaction level "READ COMMITTED". ' . $tables_missing_primary_key_text;
   ...
   +          $description .= ' ' . $tables_missing_primary_key_text;
   ...
   +        $description = 'This site is using the database transaction level "' . $isolation_level . '". This database transaction level is not supported by Drupal. The recommended database transaction level for Drupal is "READ COMMITTED".';
   

   Unless I'm mistaken, these strings cannot be translated - but should be able to be.

2. +++ b/core/modules/mysql/mysql.install
   @@ -33,11 +33,41 @@ function mysql_requirements($phase) {
   +      elseif ($isolation_level == 'READ-UNCOMMITTED' || $isolation_level == 'SERIALIZED') {
   

   Should this just be else? do we need to specify the unsupported levels?

3. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -174,6 +174,19 @@ public function getFormOptions(array $database) {
   +        'DO NOT SET BY DRUPAL' => $this->t('Do not set by Drupal'),
   

   I think this should be 'Do not set using Drupal' rather than by. Happy to hear otherwise/counter arguments.

4. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -174,6 +174,19 @@ public function getFormOptions(array $database) {
   +      '#description' => $this->t('The recommended database transaction level for Drupal is "READ COMMITTED". By selecting the option "READ COMMITTED" Drupal will set the transaction level on every page request. If possible, we recommend to set database transaction level globally. In that case select the option "DO NOT SET BY DRUPAL". For more information, see the <a href=":performance_doc">setting MySQL transaction isolation level</a> page.', [
   

   DO NOT SET BY DRUPAL is the key, not the displayed user string, we should use the displayed user string

5. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -64,12 +65,49 @@ public function testIsolationLevelWarningNotDisplaying() {
   +    // Check the message is not a warning.
   +    $this->drupalGet('admin/reports/status');
   +    $elements = $this->xpath('//details[@class="system-status-report__entry"]//div[contains(text(), :text)]', [
   +      ':text' => 'This site is using the database transaction level "READ COMMITTED". For this to work properly all tables should have a primary key. The following table(s) do no have a primary key:',
   +    ]);
   +    $this->assertCount(1, $elements);
   +    $this->assertStringStartsWith('READ-COMMITTED', $elements[0]->getParent()->getText());
   +    // Ensure it is an error.
   +    $this->assertStringContainsString('error', $elements[0]->getParent()->getParent()->find('css', 'summary')->getAttribute('class'));
   

   I think we should assert the output contains test_table_without_primary_key so we don't get a false positive here from some other future test module with a table that has no primary key

Re: 246.2

It can't be else because that would also catch $isolation_level == 'READ-COMMITTED' where there are no tables missing primary keys which should not be an error or warning.

Addressed #246. I made all the strings translatable, refactored the if statement in mysql_requirements(), simplified the descriptions a bit, changed the "Do not set by Drupal" text to "Use database default", and updated the tests to match.

All remarks of @larowlan have been addressed.
Back to RTBC.

Re-read the patch end to end and also the interdiff in #248. I think this is in a good spot, and it's good timing to get it into 10.1.x before the alpha.

Fixed these two nits on commit:

1. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
   @@ -301,6 +301,8 @@ protected function getCredentials() {
        $connection_options = array_intersect_key($connection_options, $form + $form['advanced_options']);
   +    // Remove isolation_level since that options is not configurable in the UI.
   +    unset($connection_options['isolation_level']);
   

   option

2. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/FilePathTest.php
   @@ -127,6 +127,8 @@ public function testFilePath(string $file_private_path, string $file_public_path
        $connection_options = array_intersect_key($connection_options, $form + $form['advanced_options']);
   +    // Remove isolation_level since that options is not configurable in the UI.
   +    unset($connection_options['isolation_level']);
   

   option

Did my best with commit credits, but that's challenging with 250 comments from dozens of people, so apologies for anything overlooked.

Committed/pushed to 10.1.x, thanks!

Tagging for release notes.

Great that this is fixed!
It took us only 10 years. ;-)

Thanks to everybody who made this happen!

Automatically closed - issue fixed for 2 weeks with no activity.

🐛 READ COMMITTED requirement check doesn't account for database views Needs work

Filed a follow up because the requirements check doesn't take into account how mysql treats views as a table so registers warning/error if you have any in your database because a view can't have a primary key.

Quick followup 🐛 Errors: The following table(s) do not have a primary key: forum_index Needs review

Hello everyone,

My website currently still uses the REPEATABLE READ transaction isolation level.
I would like to switch to READ COMMITED.
I read this documentation :
https://www.drupal.org/docs/getting-started/system-requirements/setting-... →
From what I understand, for best performance "The preferred way to change the transaction isolation level" is to set the Global value on the server side with
SET GLOBAL transaction_isolation='READ-COMMITTED';
However, if I'm not mistaken, this global variable is shared between all databases of the server. Plus updating this variable needs to be done with a super user.
Where I work, we have multiple databases of different websites on the same server. Plus my database user cannot update this variable. So if I decided to update this variable globally, I would need to coordinate with all other websites and ask IT to do it for me.

For that reason, updating it in settings.php would be easier for me.
In the documentation, the syntax presented in settings.php is the following:

'init_commands' => [
    'isolation_level' => 'SET SESSION tx_isolation=\'READ-COMMITTED\'',
  ],

BUT there's a warning: "Adding the setting of the transaction isolation level to the init commands in the settings.php file has the disadvantage that on every page request the transaction isolation level is set. That is an extra database call for every page request!"
Since my goal here is to improve performances, after reading this, configuring this in settings.php was not an option for me anymore.

But then I realized that I actually read here that "The transaction isolation level can be set in the database settings array in setttings.php" and that "Drupal now sets the READ COMMITTED transaction isolation level by default for new installs on MySQL."

So I wondered… if setting the isolation level inside settings.php is bad for performance because it redefines the level for each db request, how come Drupal now sets it by default inside settings.php??

I actually created a brand new Drupal website recently, and it uses indeed READ COMMITED. So I went to the settings.php but to my surprise, the way the isolation level is defined is not

'init_commands' => [
    'isolation_level' => 'SET SESSION tx_isolation=\'READ-COMMITTED\'',
  ],

but simply :
'isolation_level' => 'READ COMMITTED',

Here are my many questions:

Is the documentation outdated?

About this new syntax ('isolation_level' => 'READ COMMITTED',): does it have better performance than the 'init_commands' SET SESSION tx_isolation=\'READ-COMMITTED\ syntax? Or is it just a simpler way to write it but in the end it does the exact same thing and has the same drawback of setting the isolation level for each session?

How bad is it to actually set the transaction isolation level on every page request?
Should I do all I can to set the transaction isolation level globally on my database server and avoid to set it in settings.php whenever possible? Or I should not care about this because I won't see a difference...?

In settings.php, is the parameter 'isolation_level' => 'READ COMMITTED', added to every new Drupal site? Or is there a check that is made to the server at site creation and 'isolation_level' => 'READ COMMITTED' is added to settings.php only if the global isolation level of the server is not already READ COMMITTED?

What happens if the parameter 'isolation_level' => 'READ COMMITTED' is still present on settings.php but the DB serveur global isolation level is updated to READ COMMITED later on. Will the isolation level still be redefined for each session no matter what, for nothing? In other words, once we set READ COMMITED globally on server side, should we remove the 'isolation_level' => 'READ COMMITTED' line from settings.php to improve performances?

Thank you in advance for your feedback :)

I don't get it working.
Status report on an existing project says:
Transaction isolation level REPEATABLE-READ The recommended level for Drupal is "READ COMMITTED"
And I would like to fix that.

I cannot ron the mysql command on my hosting (this is done via phpmyadmin):
SET GLOBAL transaction_isolation='READ-COMMITTED';
Because this results in:
Query failed. Rerun with --debug to see any error message. ERROR 1227 (42000) at line 1: Access denied; you need (at least one of) the SUPER privilege(s) for this operation

I also not use the Config File method, there is no my.cnf file that I'm allowed to mess with on my hosting.

The Command Line method results in the same error as when I run the mysql command via phpmyadmin.

So that only rests me with the settings.php method.
I thought: lets check performance, change the setting, and check performance again when its changed.
But I cannot seem to get it working via the settings file.

At first I tried this, because it was mentioned in a previous comment and also on this website:

$databases['default']['default'] = array (
  'database' => 'xxxxxxxxxx',
  'username' => 'xxxxxxxxxx',
  'password' => 'xxxxxxxxxx',
  'prefix' => '',
  'host' => 'xxxxxxxxxx',
  'port' => '3306',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'isolation_level' => 'READ COMMITTED',
);

After clearing caches, the status report still showed:
Transaction isolation level REPEATABLE-READ The recommended level for Drupal is "READ COMMITTED"

So I changed settings to ( source → ):

$databases['default']['default'] = array (
  'database' => 'xxxxxxxxxx',
  'username' => 'xxxxxxxxxx',
  'password' => 'xxxxxxxxxx',
  'prefix' => '',
  'host' => 'xxxxxxxxxx',
  'port' => '3306',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'init_commands' => [
    'isolation_level' => 'SET SESSION transaction_isolation=\'READ-COMMITTED\'',
  ],
);

BUT after clearing caches, the status report still showed:
Transaction isolation level REPEATABLE-READ The recommended level for Drupal is "READ COMMITTED"

So I slightly modified and changed settings to ( source → ):

$databases['default']['default'] = array (
  'database' => 'xxxxxxxxxx',
  'username' => 'xxxxxxxxxx',
  'password' => 'xxxxxxxxxx',
  'prefix' => '',
  'host' => 'xxxxxxxxxx',
  'port' => '3306',
  'namespace' => 'Drupal\\Core\\Database\\Driver\\mysql',
  'driver' => 'mysql',
  'init_commands' => [
    'isolation_level' => 'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED',
  ],
);

BUT after clearing caches, the status report still showed:
Transaction isolation level REPEATABLE-READ The recommended level for Drupal is "READ COMMITTED"

I even checked in phpmyadmin using query:
show variables WHERE Variable_name LIKE "%_isolation";
And it shows

transaction_isolation   REPEATABLE-READ
tx_isolation                REPEATABLE-READ

And at this point I am completely lost on how to fix this / get rid of this error.