lando / pantheon

The Official Lando Pantheon plugin.
https://docs.lando.dev/pantheon
GNU General Public License v3.0
11 stars 18 forks source link

Change transaction isolation to 'READ COMMITTED' #246

Open duncancm9 opened 4 months ago

duncancm9 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

This might work:

[mysqld]
transaction_isolation = READ-COMMITTED
ethangeorgi commented 4 months ago

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.

xurizaemon commented 4 months ago

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:

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 https://github.com/lando/pantheon/pull/248#issuecomment-2132674029

ethangeorgi commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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