Warning when mysql is not set to READ-COMMITTED

Wouldn'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.

I 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.

Phone hijacked status

Yep, that node is the main place for 1.x

Looks good when I ran it through Simplytest.me

However, we should probably link to the documentation from here, too.

RTBC+1...Works like a charm. Fyi warning disappears when the setting is added to settings.php. I assume this is intentional.

Should this issue be moved to D8?

I think this would get greater tracking in the new core status page. Shall we move queues?

Let's see if anyone objects to that.

Not sure of the component

Hmm, the wider Drupal context is complicated.

Discussed 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:

  $databases['default']['default']['init_commands'] = ['tx_level' => 'SET TRANSACTION ISOLATION LEVEL READ COMMITTED;']

For 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:

^ array:1 [
  "@@tx_isolation" => "READ-COMMITTED"
]

@froboy: Thanks, that's really helpful and surely saved me from some WTF confusion.

I'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:

<?php

namespace Drupal\Tests\mysql\Functional;

use Drupal\Core\Database\Connection;
use Drupal\Core\Database\Database;
use Drupal\Tests\BrowserTestBase;

class RequirementsTest extends BrowserTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['mysql'];

  /**
   * {@inheritdoc}
   */
  protected $defaultTheme = 'stark';

  /**
   * {@inheritdoc}
   */
  protected function setUp(): void {
    parent::setUp();

    // The isolation_level option is only available for MySQL.
    $connectionInfo = Database::getConnectionInfo();
    if ($connectionInfo['default']['driver'] !== 'mysql') {
      $this->markTestSkipped("This test does not support the {$connectionInfo['default']['driver']} database driver.");
    }

  }

  public function testRequirementsStatusPage() {
    $admin_user = $this->drupalCreateUser([
      'administer site configuration',
      'access site reports',
    ]);
    $this->drupalLogin($admin_user);

    $session = $this->assertSession();
    $connection = parent::getDatabaseConnection();

    // Check the warning message for the isolation level.
    $this->drupalGet('admin/reports/status');
    $this->assertSession()->pageTextContains('Database Isolation Level');
    $elements = $this->xpath('//details[@class="system-status-report__entry"]//div[contains(text(), :text)]', [
      ':text' => 'Drupal can suffer from deadlock issues related to fields.',
    ]);

    $this->assertCount(1, $elements);
    $this->assertStringStartsWith('Transaction Isolation Level', $elements[0]->getParent()->getText());

    // Update the transaction level.
    $connection->query("SET TRANSACTION ISOLATION LEVEL READ COMMITTED")->execute();

    // Make sure the warning message is gone.
    $this->drupalGet('admin/reports/status');
    $session->pageTextNotContains('Database Isolation Level');

  }

}

@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.

Thanks 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.

+++ b/core/modules/mysql/tests/src/Functional/IsolationLevelSettingTest.php
@@ -0,0 +1,67 @@
+    \Drupal::service('module_installer')->install(['mysql']);

You'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.

1. +++ b/core/modules/mysql/tests/src/Functional/IsolationLevelSettingTest.php
   @@ -0,0 +1,67 @@
   +class IsolationLevelSettingTest extends UpdatePathTestBase {
   

   We 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.

2. +++ b/core/modules/mysql/tests/src/Functional/IsolationLevelSettingTest.php
   @@ -0,0 +1,67 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function prepareSettings() {
   +    parent::prepareSettings();
   +
   +    // Set sql_require_primary_key for any future connections.
   +    $settings['databases']['default']['default']['init_commands'] = (object) [
   +      'value'    => [
   +        'tx_level' => 'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED',
   +      ],
   +      'required' => TRUE,
   +    ];
   +
   +    $this->writeSettings($settings);
   +  }
   

   This can go in the non-upgrade test and it'll work just fine. In fact you can do

   +    $settings['databases']['default']['default']['init_commands'] = (object) [
   +      'value'    => [
   +        'tx_level' => 'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED',
   +      ],
   +      'required' => TRUE,
   +    ];
   +
   +    $this->writeSettings($settings);
   

   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.

Thanks @alexpott, I've updated the test and now I was able to test both scenarios.

Regarding #40:

1. No worries!
2. The code snippet you brought saved the test, thanks.

   One thing that is interesting is that DrupalCI's mysql is not using READ COMMITTED - I wonder what it is using.

   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.

Fixing the cspell problem

+++ b/core/modules/mysql/mysql.install
@@ -0,0 +1,46 @@
+          'description' => t('Drupal 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

', [

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?

Drupal 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>.

Thanks 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:

1. Good catch! I thought the documentation was from core, so the best here is to review #3266063: New page for database transaction isolation setting for MySQL before work on this
2. I see, I've foolowed the #29, "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", I've tried to implement both here, the link to the documentation and the fix, but maybe that's too much, just the documentation seems enough
3. It's a good idea, but I think this was implemented at the parent issue(#1650930)
4. Definitely!

Re 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.

For MySQL, MariaDB or equivalent databases the 'isolation_level' option can
be set. The recommended option is 'READ COMMITTED'. The other 3
options are 'REPEATABLE READ', 'READ UNCOMMITTED' and 'SERIALIZABLE'. They
are not supported; use them at your own risk. For more info:
https://docs page

# $databases['default']['default'][' 'init_commands']['isolation_level'] =  'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED';

My 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!

Great.

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.

+++ b/core/modules/mysql/mysql.install
@@ -0,0 +1,45 @@
+          'description' => t('For the best performance and to minimize locking issues, the READ-COMMITTED transaction isolation level is <a href=":performance_doc">recommended</a>', [

Nit: missing period at end of sentence.

I will be working on this ( commentary #50)

Just changed the content as described in #48 and #50.

+++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
@@ -0,0 +1,90 @@
+  private function writeIsolationLevelSettings(string $isolation_level) {

Nit: 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.

Don't we usually almost always use protected not private? Not sure if this matters in tests or not.

I 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.

#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...

1. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -0,0 +1,90 @@
   +// cspell:ignore commited
   

   You write "committed" with double t, not a single t.

2. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -138,6 +138,19 @@
   + * $databases['default']['default']['init_commands'][
   + *    'isolation_level',
   + * ] = 'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED';
   

   This should be:

   $databases['default']['default']['init_commands'] = [
     'isolation_level' => 'SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED',
   ]
   

3. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -138,6 +138,19 @@
   + * For MySQL, MariaDB or equivalent databases the 'isolation_level' option can
   + * be set. The recommended option is 'READ COMMITTED'. The other 3
   + * options are 'REPEATABLE READ', 'READ UNCOMMITTED' and 'SERIALIZABLE'. They
   + * are not supported; use them at your own risk. For more info:
   + * https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-levels.html
   

   Can we change this to:

    * For MySQL, MariaDB or equivalent databases the 'isolation_level' option can
    * be set. The recommended transaction isolation level for Drupal sites is 
    * 'READ COMMITTED'. The 'REPEATABLE READ' option is supported but can result
    * in deadlocks, the other 2 options are 'READ UNCOMMITTED' and 'SERIALIZABLE'.
    * They are available but not supported; use them at your own risk. For more
    * info:
    * https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-levels.html
   

   The text is then the same as on its documentation page: https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa...

4. +++ b/core/modules/mysql/mysql.install
   @@ -0,0 +1,45 @@
   +    if (Database::isActiveConnection() && (Database::getConnection()->driver() == 'mysql')) {
   

   This is now part of the mysql module, therefor the part with driver() == 'mysql' can be removed.

5. +++ b/core/modules/mysql/mysql.install
   @@ -0,0 +1,45 @@
   +            ':performance_doc' => '',
   

   The link to the documentation page is: https://www.drupal.org/docs/system-requirements/setting-the-mysql-transa...

6. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -0,0 +1,90 @@
   +    $this->assertSession()->pageTextNotContains('Database Isolation Level');
   

   Can we copy this test to after the previous page call and assert that the text exists.

7. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -0,0 +1,90 @@
   +        'tx_level' => "SET SESSION TRANSACTION ISOLATION LEVEL {$isolation_level}",
   

   Nitpick: Can we change "tx_level" to "isolation". It is what we want to use in the followup issue.

Addressed 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!

The 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.

@daffie, I'll check this...

I'm sorry, I chose the wrong status

All 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!

Couple of nits but otherwise looks great.

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -138,6 +138,21 @@
   + * 'READ COMMITTED'. The 'REPEATABLE READ' option is supported but can result
   + * in deadlocks, the other 2 options are 'READ UNCOMMITTED' and 'SERIALIZABLE'.
   + * They are available but not supported; use them at your own risk. For more
   + * info:
   

   Nit, should be 'two' instead of '2'.

2. +++ b/core/modules/mysql/tests/src/Functional/RequirementsTest.php
   @@ -0,0 +1,88 @@
   +
   +  /**
   +   * Write the isolation level on settings.php.
   +   *
   

   Should be 'Writes the isolation level in settings.php".

Addressed the points at #64.

The remarks of @catch have been addressed.
Back to RTBC.

Committed/pushed to 10.0.x and 9.5.x, thanks!

I think that there are a couple of issues with this patch:

• We say REPEATABLE READ is supported but we make a warning on the status report
• We add words to the status report value that are the same as the title - that's not how this is supposed to work
• We remove the message when the value is set to a supported value - I think we should be setting the severity to OK. Having this info around is still handy.

Opened #3291949: Improve the MySQL transaction isolation warning to address this.

I have published the CR and I have updated the documentation page to reflect the changes in the CR.