search

lando / pantheon

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

Adding transaction isolation #248

Open mikeethedude opened 6 months ago

mikeethedude commented 6 months ago

Purpose

resolves: #246

In Drupal 10+ we need the transaction isolation to be set to READ-COMMITTED. Adding this to the mysql.cnf seems to be the most direct route to this. I believe if this isn't supported in older versions of maria this will be ignored as much other config is ignored when correctly assembled even if it doesn't apply.

Testing:

netlify[bot] commented 6 months ago

Deploy Preview for lando-pantheon failed. Why did it fail? →

Name Link
Latest commit 43c64c0f4cc0faaaac110227f8ac389d857dbf2e
Latest deploy log https://app.netlify.com/sites/lando-pantheon/deploys/6650dfdbaf80b40008796118
mikeethedude commented 6 months ago

Additional notes here:

I've also tried this by dropping a new project in the drupal10 example directory in this.

Connecting to a pantheon site locally here.

name: test-app
recipe: pantheon
config:
  composer_version: 2
  framework: drupal10
  site: site_name
  id: pantheon_site_id
xurizaemon commented 5 months ago

Are you able to provide a reference for any requirement that Drupal requires the MySQL server to have this configuration?

Refer Drupal's DB server requirements:

Which says

MySQL, MariaDB, or Percona Server (Recommended)

Drupal 10

  • MariaDB 10.3.7+
  • MySQL/Percona 5.7.8+

Drupal 11

  • MariaDB 10.6
  • MySQL/Percona 8.0

Required configuration

  • InnoDB as the primary storage engine
  • The PDO database extension

Note: Drupal itself will generally operate with a default MariaDB/MySQL configuration. A more complex site will likely require configuration changes for the database. You can solve these common configuration issues using this documentation.

My understanding is that Drupal's DB connection should be configured for this, per https://www.drupal.org/docs/getting-started/system-requirements/setting-the-mysql-transaction-isolation-level - but I don't know that a MySQL server configuration is required here.

If the MySQL/MariaDB defaults work, I recommend to stick with them as much as possible. If there's a Pantheon or Drupal requirement for the server configuration, let's document that here was part of the reason for introduction.

Only if we know that Pantheon's DB server configuration matches the configuration here, then making this change to match to the Pantheon plugin makes sense to me.

xurizaemon commented 5 months ago

Hmm, if from 10.1.x Drupal has READ-COMMITTED as the default (#1650930), would this be necessary? Maybe the server config drupal.org docs may be due for an update instead?

https://www.drupal.org/project/drupal/issues/1650930#comment-14580623 (2022) further says that it looks like Pantheon has this setting as READ-COMMITTED - if so then yeah the proposed change seems correct for Pantheon plugin.

reynoldsalec commented 5 months ago

This underscores the need to add in a pantheon-default or similar test suite that just runs through all the default config to check for things like this, as we did on Acquia: https://github.com/lando/acquia/tree/main/examples/acquia-default. That would make testing this trivial.

mikeethedude commented 5 months ago

Yeah near as I can tell for an existing site, this server level change would be required to avoid seeing the message on the status page. Not having this would also potentially block a Drupal upgrade when working on this locally. I'm open to other ways to approach this though.

reynoldsalec commented 5 months ago

@AaronFeledy did you say you were going to take a look at adding some "defaults" tests around this? Know you mentioned something about it last week but wanted to doublecheck before I looked back at it...

AaronFeledy commented 5 months ago

@reynoldsalec Haven't had a chance to dig in yet so feel free to take over.