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

Change transaction isolation to 'READ COMMITTED' #246

Open
duncancm9 opened this issue May 17, 2024 · 11 comments · May be fixed by #248
Open

Change transaction isolation to 'READ COMMITTED' #246

duncancm9 opened this issue May 17, 2024 · 11 comments · May be fixed by #248

Comments

@duncancm9
Copy link

duncancm9 commented May 17, 2024

As per:

https://www.drupal.org/docs/getting-started/system-requirements/setting-the-mysql-transaction-isolation-level

The recommended transaction isolation level for Drupal sites is 'READ COMMITTED'. The 'REPEATABLE READ' option is supported but can result in deadlocks; the other options are 'READ UNCOMMITTED' and 'SERIALIZABLE'. They are available but not supported; use them at your own risk.

The current transaction isolation level being set is REPEATABLE READ. On Pantheon environments it is set to READ COMMITTED

@ethangeorgi
Copy link
Collaborator

Can confirm. Checked two of my Pantheon/Drupal sites, and the Lando sites have a warning about this in the Drupal Status Report.
Transaction isolation level REPEATABLE-READ
This hasn't caused a problem, since this isn't in production, but I'm interested in looking into it.

@reynoldsalec
Copy link
Member

Updating the default mysql.conf in the recipe seems like the way to go, although the MariaDB docs have me a little confused...says that transaction_isolation is only an applicable value in 23.07/08 Enterprise, but the tx_isolation docs say you should set the transaction_isolation value in cnf 🤷

@ethangeorgi
Copy link
Collaborator

ethangeorgi commented May 23, 2024

Alright, here's what I've been trying today.

lando mysql and SHOW GLOBAL VARIABLES LIKE 'tx_isolation';
tells me tx_isolation is REPEATABLE-READ as expected. I can use
SET GLOBAL tx_isolation='READ-COMMITTED';
to change that, and Drupal Status Report is happy with that. But that's not "permanent." If you have to lando rebuild it resets.

I edit ~/.lando/plugins/@lando/pantheon/config/mysql.cnf
Lando appears to copy this to ~/.lando/config/pantheon/mysql.cnf
I see my changes in both places.

lando rebuild does not change the tx_isolation. And this is the goal.

I've added

[mariadb]
transaction_isolation=READ-COMMITTED

in several variations. Commenting out the [mariadb] group, putting quotes around READ-COMMITTED, using tx_isolation, nothing works.

lando info tells me it's using the cnf file I expect.
Docker tells me the db is using the cnf file I expect.

Am I in the correct place?

@AaronFeledy
Copy link
Member

AaronFeledy commented May 23, 2024

This might work:

[mysqld]
transaction_isolation = READ-COMMITTED

@ethangeorgi
Copy link
Collaborator

Putting it under [mysqld] doesn't help. Nor [mysql] without the d. Nor the spaces.

According to https://mariadb.com/kb/en/set-transaction/ it's transaction-isolation with a dash not an underscore, so I tried that. No luck there either. Will keep tinkering.

@mikeethedude mikeethedude linked a pull request May 24, 2024 that will close this issue
7 tasks
@xurizaemon
Copy link

xurizaemon commented May 27, 2024

I believe adjusting the server configuration might be the wrong layer to address this. The application configuration can be made in settings.php

Addressing it at the server might seem convenient, but can also have impact outside of the intended change, eg:

  • Applying this change in the development environment's DB server configuration creates a setup for an application to appear as though it is correctly configured, until deployed to a non-Lando hosting environment where it would lack the settings.php configuration recommended
  • Applying this change in the Pantheon plugin will apply the requirement to WordPress development environments on Lando/Pantheon, which is outside the scope of the requirement.

The linked Drupal docs do suggest the server configuration as preferred, but for safety's sake I'd take that to refer to the hosting environment, not development environments. To my mind this is best considered an application concern.

If we know that Pantheon's DB server configuration matches the configuration in #248, then making a matching change to the Pantheon plugin makes sense.

Ref #248 (comment)

@ethangeorgi
Copy link
Collaborator

Interesting.

I see that in default.settings.php it has a section for setting the transaction isolation level. I cannot see how Pantheon does that, but they might have hidden config files in the dev/test/live environments that don't show up in the codebase.

The WordPress site I have on Pantheon does have transaction isolation level set to READ COMMITTED and I have no idea how that's done either. (WordPress is not my strength anymore.)

So, looking into changing something related to settings.php ... it looks like that file loads a settings.local.php if that file exists. The .gitignore file lists this file specifically, so you can't push it up to the the dev/test/live environments (unless you try really hard) so it won't break anything in production.

If I drop the following into that (new to me) file and lando rebuild I get what we're looking for.

<?php

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

The tx_isolation is READ COMMITTED and the warning goes away from the Drupal Status Report.

I'm not sure if this is a file the pantheon recipe should created/modify. I don't think I understand enough about that. But hopefully this information will help.

@AaronFeledy
Copy link
Member

READ COMMITTED does appear to be the platform-wide default on Pantheon, which is a completely reasonable default for any version of Drupal or Wordpress.

@ethangeorgi
Copy link
Collaborator

Okay, mucked around with this for a few hours, thinking the mysql.cnf thing should work but does not, so is there another, less perfect way we can get this done?

In /pantheon/scripts/pull.sh around line 170 it does "some post db things," so I thought maybe I can stuff it there. In front of that I put

  # Do some post DB things to set the transaction isolation level
  echo "Setting your transaction isolation level..."
  mysql --user=root --host=database --port=3306 -e "SET GLOBAL tx_isolation='READ-COMMITTED';"

The idea being, we just imported the db, let's set the transaction isolation level with SQL. And it works. You don't even have to rebuild or destroy, just another pull and it works. (Among other checks, in the Drupal Status report the warning is gone.)

So my question is, is this an acceptable way to do this? If so, I'll submit a proper PR.

@reynoldsalec
Copy link
Member

Certainly seems better than nothing, although it bothers me that there isn't a way to set it in a universal fashion that would account for other DB import strategies.

I'm guessing you've tried every variant in the book @ethangeorgi, but was this (config suggested to me by the almighty AI) one of them?

[mysqld]
transaction-isolation = READ-COMMITTED

@ethangeorgi
Copy link
Collaborator

No luck with that either. Tried rebuild and a destroy and start for good measure.

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 a pull request may close this issue.

5 participants