Use READ COMMITTED by default for MySQL transactions

Isn't this rather edge-case-y for a major? Apologies in advance if I'm wrong.

Otherwise, this is in line with other uses of db_transaction().

CNW for tests.

If for some reason the db_merge() query fails, the current transaction is rolled back.

It should not. We start a transaction in MergeQuery::execute(). Not sure what you are describing, but it's definitely not the expected behavior.

Do you have a way to reliably reproduce the issue?

Well, if you have MySQL 5.1 then sure http://harrison-fisk.blogspot.ca/2009/02/my-favorite-new-feature-of-mysq... you want READ COMMITTED. But we require http://drupal.org/requirements/ only 5.0.15. Is http://ronaldbradford.com/blog/understanding-mysql-innodb-transaction-is... this situation a problem?

This is pretty scary: http://bugs.mysql.com/bug.php?id=45357

I wouldn't go READ_COMMITTED for InnoDB, the default (REPEATABLE READ) is WAY more tested. That being said, if you just want READ COMMITTED for a particular operation, just using it for that operation should be fine.

Things have evolved since last year. It is now clear that running a reasonably busy Drupal 7 website under REPEATABLE READ is just suicide. Drupal.org (and several other busy sites that I know of) are successfully running on READ COMMITTED.

I'm marking #2164849: Enforce READ COMMITTED transaction isolation level as a duplicate, here is what I said there:

Our merge query implementation and (part of) the lock implementation) only work properly under READ COMMITTED transaction isolation. We could decide to enforce this, but in the meantime, just set transaction-isolation=READ-COMMITTED in your MySQL server configuration, or add the runtime mentioned by @gielfeldt above.

I know several high-performance sites (including Drupal.org) that are using this successfully.

Recently switched to READ COMMITTED as well and things are looking a lot better on our setup.

From my last comment on that dup thread:
Thinking we should set this as the default for all new Drupal installs on MySQL.

$databases['default']['default']['init_commands'] = array(
  'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'"
);

Do we want to put the code in the install form (install_settings_form) as a hidden field or drop it in at a later stage (install_settings_form_submit)?

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -139,6 +139,13 @@ class DatabaseBackend implements CacheBackendInterface {
+      $transaction = db_transaction();

this should use injected $this->connection

We tested READ-COMMITTED level and it eliminated deadlocks but we noticed when using replication, it's possible that replication falls behind and gets out of sync without being able to catch up. Has anyone else run into this? I'm not sure if this situation has been considered based on wanting to make this a default in #10. Thanks.

We have 1 slave for replication and have not see any issues with it falling behind so far.

In a replicated MySQL environment, using READ-COMMITTED isolation requires using row-based replication and it's critical to have PRIMARY or UNIQUE keys in all your tables that you change. When you have InnoDB tables without primary keys, and use ROW based replication then you'll hit this bug: http://bugs.mysql.com/bug.php?id=53375. We observed large replication delays and it was just when load testing a single database that was set to READ-COMMITTED (one of many on the database server). We found some tables without primary keys:

taxonomy_index
form_builder_cache
config_builder_index
zipcodes
location_instance
nodequeue_types
nodequeue_roles
nodequeue_nodes

Query to find tables without a primary or unique key from the comments of this article http://datacharmer.blogspot.com/2011/09/finding-tables-without-primary-k...

SELECT 
  tables.table_schema, 
  tables.table_name,
  -- tables.table_rows,
  tables.engine
FROM information_schema.tables AS tables
LEFT JOIN (
  SELECT table_schema, table_name
  FROM information_schema.statistics
  GROUP BY table_schema, table_name, index_name
  HAVING
  SUM(
    CASE
    WHEN non_unique = 0
    AND nullable != 'YES' THEN 1
    ELSE 0
    END
  ) = COUNT(*)
) AS statistics
  ON tables.table_schema = statistics.table_schema
  AND tables.table_name = statistics.table_name
WHERE statistics.table_name IS NULL
AND tables.table_type = 'BASE TABLE'
AND tables.engine != 'PERFORMANCE_SCHEMA'
-- AND tables.table_rows > 0

Tables that have issues on my site:

flag_types
taxonomy_index
views_data_export_object_cache
wysiwyg_user
zendesk_users

taxonomy_index is in core so that should be fixed. Running this query on this table shows the duplicate entries

SELECT 
  ti.counter,
  taxonomy_index.*
FROM taxonomy_index AS taxonomy_index
INNER JOIN (
  SELECT 
    COUNT(*) AS counter,
    ti.*
  FROM taxonomy_index AS ti
  GROUP BY ti.nid, ti.tid
  HAVING counter > 1
  ORDER BY counter DESC, ti.nid ASC
) AS ti
  ON ti.nid = taxonomy_index.nid
  AND ti.tid = taxonomy_index.tid

On my database, by looking at the created column it appears that this is an error as the created column has the same date across all rows where the nid and tid are the same.

In this case for the core table using ALTER IGNORE TABLE seems like the best way forward.

Also noted that I keep getting deadlocks on the cache_field when cid = field_info_types:en. I created an issue for this #2193149: Deadlocks occur in cache_field table..

Also noted that the semaphore table has issues as well. Using a MEMORY table and everything seems good once I switched to BTREE #1898204-17: Do not use InnoDB for the semaphore table, use Memory

With the 2 changes in the above issue, it has been 24 hours since the last deadlock according to SHOW ENGINE innodb STATUS which is a first for us in a long time.

So what is a proposed solution here?
Seems better to add this to default database settings line #10 suggests

I would like to go forward with #10; if no one has any suggestions on where to put this I'll put in in the advanced options section inside DatabaseTasks::getFormOptions

Also noted #2222635: Waiting for table metadata lock on cache_field table. With this issue I'm opting for table rename without the drop.

Patch needs a little bit of cleanup and we might only want to show that field when the database is of the type mysql.

Made patch against 7.x; will flip back to 8.x once the test passes.

20: drupal-1650930-20-read-committed.patch queued for re-testing.

while adding an initial connection string as a configuration option may seem reasonable I've got two reservations
a) The mysql default of REPEATABLE-READ makes more sense, especially in the merge function, so i'd suggest a blank value here as default.
b) as a global setting you don't know what assumption other modules/core bits have assumed (but they shouldn't assume),
b) Isn't setting this on a DB global place like my.cnf more appropriate (sure its not accessible if you don't manage the server).

If you want to select a transaction isolation level it probably should be specified in the db_transaction arguments (it won't nest however).

My gut feel is the db_merge function should be done at SERIALIZABLE isolation level to prevent updates between the select and insert implementation in merge. (and this function should be implemented as a INSERT OR REPLACE for mysql/sqlite where keys are primary or unique keys).

From http://www.mysqlperformanceblog.com/2013/12/12/one-more-innodb-gap-lock-...

So you see how easy it is to cause a deadlock, so make sure to avoid this situation – if a non-INSERT write operation is more likely to not match any row because the INSERT part is yet to come on the transaction, don’t do it or use REPLACE INTO or use READ-COMMITTED transaction isolation.

Replace does sound like it could work and it looks like it was never discussed in #937284: DEADLOCK errors on MergeQuery INSERT due to InnoDB gap locking when condition in SELECT ... FOR UPDATE results in 0 rows

db_merge requires stronger isolation than REPEATABLE READ in order to fix deadlocks or did I miss something? http://www.ovaistariq.net/597/understanding-innodb-transaction-isolation... If you could explain this recommendation in detail and weigh the pro's and con's that would be ideal.

Thanks for creating #2232037: taxonomy_index should have a primary key :) Also feel free to give advice on these sets of recommendations: https://groups.drupal.org/node/415883

it seems the db cache was worked around in #1167144: Make cache backends responsible for their own storage that performs retries on exceptions. The deadlock was most likely an insert of two identical cache entries at the same time. This will be fixed in #2222635: Waiting for table metadata lock on cache_field table once the cid (or hash there of) isn't a primary (and therefore unique) key.

In essence deadlocks are a consequence of databases not being able to meet the ACID requirements within the relaxed to strict requirements that the isolation level provides. The way to deal with the deadlock really needs to be handled by the application and in the case of cache inserts a retry is appropriate.

I'd suggest the transaction isolation level should be an argument to the db_transaction object that reverts on commit.

I suggest we lower the default transaction level at the BDTNG level as a first step and start thinking how about we need ACID in Drupal as a follow-up. We have to take into account that Drupal database schema is stupid (very very stupid) and doesn't comply to any king of consistency since it doesn't even have any FOREIGN KEY constraints. It doesn't have any constraints at all except PRIMARY KEYs and some sporadic UNIQUE KEY. Let's avoid stupid deadlocks.

> I suggest we lower the default transaction level at the BDTNG level

From what I've seen of the code it seems to me making a lot of assumptions that a SELECT statement is a perfect consideration of a following UPDATE (e.g. db_merge) and as such assuming a default REPEATABLE-READ is in effect.

Helper fixes that allow things to be fixed after careful consideration:

• #2236395: extended db_transaction / Database::Connection::startTransaction to include isolation level for transaction isolation levels.
• #1800286-16: Update, rather than Delete and insert - for significant database performance improvement

> Drupal database schema is stupid (very very stupid) and doesn't comply to any king of consistency since it doesn't even have any FOREIGN KEY constraints.

Ok, lets find the instances of these, add a key and and put them in a transaction so at least its an all or nothing change.

> It doesn't have any constraints at all except PRIMARY KEYs and some sporadic UNIQUE KEY. Let's avoid stupid deadlocks.

If you define the constraints of PRIMARY and UNIQUE you shouldn't be surprised if occasionally parallel requests cause inserts of the same primary/unique key. Its not the database problem to say which is right or wrong. It just accepts one and rejects another with an exception.

So lets also avoid dumb deadlock detection and processing (core/lib/Drupal/Core/Cache/DatabaseBackend.php:set where its retried without realizing that its happening because the cid is unique and two threads failed a cache hit at the start and separately created a new entry after generating the same data. The right behaviour here is to catch the deadlock exception and ignore it and not retry a set (are the cache entries really any different?). Alternately #2222635-14: Waiting for table metadata lock on cache_field table that has cidhash as non unique).

I'm not really sure you actually understood what I said.

just a link to https://drupal.org/node/1800286#comment-8672597, where a test is made of parallel DELETE+INSERT on REPEATABLE READ -> deadlock. And delete+insert is the default mode for drupal field updates...

In reply to #14/#15 this has been committed to D8 and D7 patch is waiting #610076: Add a primary key to the {taxonomy_index} table and prevent duplicates being inserted

Does anyone have any experience running a commerce site with READ COMMITTED?

@tatyana: we have been recommending READ COMMITTED for a very long time. Virtually all the large users of Drupal Commerce are running under READ COMMITTED.

Great, thanks for the confirmation

Getting a Fatal error: Unsupported operand types

Adding this to database.inc seems to fix it

    if (!is_array($connection_options['init_commands'])) {
      unset($connection_options['init_commands']);
      $connection_options['init_commands'] = array();
    }

This needed to be adjusted in install.core.inc as well (missing the list dir).
Fatal error: Call to undefined function list_extract_allowed_values()

    include_once DRUPAL_ROOT . '/modules/field/modules/list/list.module';
    $init_commands = list_extract_allowed_values($init_commands, 'list_text', TRUE);

#610076: Add a primary key to the {taxonomy_index} table and prevent duplicates being inserted is issue to add primary to taxonomy_index

It's an edge case but one that lots and lots of sites run into quite frequently, and it's a fatal when you do.

So if you're using replication, it looks like you'd need these three (3) settings. The additional two (2) on the end are optional.

# Lock only index records, not the gaps before them.
# https://dev.mysql.com/doc/refman/5.6/en/set-transaction.html
transaction-isolation = READ-COMMITTED

# With READ COMMITTED, you must use row-based binary logging.
# https://dev.mysql.com/doc/refman/5.6/en/binary-log-setting.html
binlog_format = row

# Eliminate a race condition with row-based replication.
# https://dev.mysql.com/doc/refman/5.6/en/innodb-auto-increment-configurable.html
innodb_autoinc_lock_mode = 2

# Uncomment this to shrink the amount of data written to the log.
# https://dev.mysql.com/doc/refman/5.6/en/replication-options-binary-log.html#sysvar_binlog_row_image 
#binlog_row_image = minimal

# Uncomment this to see the queries not normally available in row format.
# Useful for debugging.
# https://dev.mysql.com/doc/refman/5.6/en/replication-options-binary-log.html#sysvar_binlog_rows_query_log_events
#binlog_rows_query_log_events = TRUE

It looks like these settings need to be on both master & slaves as per a MySQL Server Asynchronous Replication Example.

References:

• My Favorite New Feature of MySQL 5.1: Less InnoDB Locking
• Row-based Replication | Master MySQL

I'd add this to the wiki page, but Fixes for MySQL Deadlocks in D7 isn't a wiki page.

Add it as a comment; unfortunately I can't change it to a wiki. Thanks for doing more research into this :)

For some MySQL version context, we proposed changing the default to READ-COMMITTED, but later withdrew the proposal based on feedback received. Here is my blog post in it: http://www.tocker.ca/2015/01/14/proposal-to-change-replication-and-innod...

Dimitri has also published benchmarks on isolation level impact. Repeatable-read performs better with short transactions/queries:
http://dimitrik.free.fr/blog/archives/2015/02/mysql-performance-impact-o...

(I'm still a fan of read-committed.. just noting this change isn't as straight forward beneficial as it seems. Dan's comments about db_merge are also true: there are probably other reapeatable-read assumptions being made.)

This directly conflicts with #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate.

We recently encountered “Deadlock found when trying to get lock; try restarting transaction: DELETE FROM {cache_field}” in production.
we run following as part of deployment process
> drush updb -y
> drush fra -y
> drush cc all

We use replication.

Is putting following READ-COMMITTED setting in settings.php is safe in production environment (or) we should do at mysql server level as mention by https://www.drupal.org/node/1650930#comment-9407649

$databases['default']['default']['init_commands'] = array(
'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'"
);

We have also found this issue on Drupal 8 (we tested using 8.0.2), on user registration flow (URL: /user/register or using directly the User functions in core).
The deadlock occurs with the 'users_field_data' table.

In our initial tests, these deadlocks have stopped after changing to 'READ-COMMITTED'.

Postponing on #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate.

Bumping to 8.2.x.

FYI: MySQL made READ-COMMITTED its new default in version 5.7.

@Wim Leers: https://dev.mysql.com/doc/refman/5.7/en/innodb-transaction-isolation-lev... says the default is still REPEATABLE READ.

Lowering the default transaction level per default is often a bad idea, in theory, if database were fast enough, it should always remain isoled serialisable. I experienced very bad behaviors with commerce due to read commited level that left lots of data in an incoherent state. It should be left to each site developer or infrastructure administrator to decide such a dangerous choice.

#54: you're right, I misread http://www.tocker.ca/2015/01/14/proposal-to-change-replication-and-innod...! That was only a proposal, it was not actually applied.

#55: Very interesting remark! Because Damien Tournoud wrote at #1650930-35: Use READ COMMITTED by default for MySQL transactions:

we have been recommending READ COMMITTED for a very long time. Virtually all the large users of Drupal Commerce are running under READ COMMITTED.

(He wrote that precisely 4 years ago today!)

We ran into these Deadlocks. I think that READ COMMITTED should be the default isolation level for drupal in general.
In fact sub-systems like Redis, Memcache, APCU, Solr, elastic search, ... are defacto opperating in a "READ COMMITTED" mode and therefore MySQL / MariaDB should be configured consistently. Any code in drupal that relies on REPEATABLE READ should be considered to be wrong.

> In fact sub-systems like Redis, Memcache, APCU, Solr, elastic search, ... are defacto opperating in a "READ COMMITTED" mode

Those are not RDBMS, lowering the default transaction level may also cause data inconsistency issues.

If you run REPEATABLE READ you get a completely different behavior if you store a cache entry during a transaction within the database or for example apcu (which the core enables automatically for some caches if available). With READ COMMITTED you get the same behavior. That's why I argue that any code that relies on REPEATABLE READ needs to be considered to be wrong.

Before Drupal switched to InnoDB and used MyISAM, we defacto had something like READ COMMITTED, too (READ UNCOMMITTED to be precise).

The Deadlock we face at the moment causes a data loss unless you write some code that re-runs the latest request.

We made significant changes to router rebuilding, such that I'm not sure #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate is an issue any longer. So unpostponing this.

Can we just set the default init_commands like this?

I think that the approach taken in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate to improve transaction support in our abstraction layer is better than explicitly setting an isolation level. We would want to allow more complex approaches to transactions as it is really, really limited at the moment, and it makes sense to me to allow developers to change the behavior based on complex database models.

it makes sense to me to allow developers to change the behavior based on complex database models.

@mradcliffe I disgree. As I already stated in #61, any contrib or core module that requires REPEATABLE READ does something wrong and the code isn't portable to another database or cache backends anymore.

If your custom module requires it, you already can set it using the existing low level mechanisms.

Therefore I don't think that an API like proposed in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate should be introduced.

I disagree with the following part of your statement that "code isn't portable to another database", @mkalkbrenner. And the differences with transaction isolation level behavior is precisely WHY a developer may want READ COMMITTED over REPEATABLE READ (or SERIALIZABLE over REPEATABLE READ). Furthermore the entire point of a database abstraction layer (DBAL) is to have a high-level implementation to give us tools to do so in code rather than mucking about directly in SQL. In my opinion this leads to a maintainability nightmare of SQL scripts in custom projects.

The API in #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate also does more than just introduce isolation levels, but attempts to bring Drupal's transaction handling in par with other DBAL such as Doctrine. Because we don't have robust transaction handling we ended up with this weird Storage API that explicitly depends on implicit transaction commits, which made it necessary to write a stupid hack around it to get things to work in the pgsql driver.

A robust transaction interface that allows for better transaction commits, rollbacks and isolation levels

1. Makes it easier for core and contrib. database drivers to implement transaction isolation levels i.e. improves our core/contrib db drivers.
2. Improves Developer Experience by allowing custom development to use a common interface i.e. improves maintainability.
3. Helps put our current DBAL on a better playing field i.e. improves marketability.

I don't see how not providing a common interface for developers helps out in the long term.

It is a bit off-topic, and so I think that we should add the #2572283 as a follow-up todo in the patch in #63.

Good idea, added the @todo.

The test run took 48 minutes vs. 49 minutes on 8.7.x-dev, so we might see a tiny performance improvement with this :-)

We still tell all Commerce sites to run under "READ COMMITTED" (and there's even a patch for adding a warning when it doesn't: #2733675: Warning when mysql is not set to READ-COMMITTED). It's also the first step of every performance engagement we do for a client.

Can't comment on the problems hinted at in #55, we haven't encountered them so far.

I think this issue could use a new summary.

In particular, a list of pros/cons based on what we currently know about Drupal sites running READ COMMITTED or having tried to do so but switched back due to problems with it. For example:

• drupal.org and Commerce sites (both are cases of write-heavy sites) seem to be good success stories so far. Any other examples where it's working out well?
• Does it cause known problems for sites that are read-heavy but not write-heavy? Are there any specific examples of sites for which READ COMMITTED leads to data integrity failures?

Good news: I applied the patch from #67 to my CI setup and ran the Bash script from the related issue #2833539. The script is just a quick way to reproduce an otherwise intermittent issue. With the Bash script, I used to get tons of these errors "1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {node_field_data}". Thanks to the patch, they are gone. I switched between vanilla Drupal 8.6.10 and with patch applied multiple times and found that the patch consistently fixed my issues.

Without the patch, our CI pipeline quickly fails. With the patch, it consistently succeeds.

My setup: Ubuntu 18.04.1, Drupal 8.6.10 running within Docker via Lando v3.0.0-rc.13, MySQL 5.7.25 within Docker, all with default settings.

Does it cause known problems for sites that are read-heavy but not write-heavy?

I don't see how this could be an issue, the transaction level is going to have less impact the less writes there are.

I deliberately did not get this on my local machine, and I run into it on almost every cache clear. Bumping to critical since it's a fatal error during normal site operation that affects regular users.

For other interested parties reading this issue and wondering about taxonomy_index: in D8 it has a primay key. It was added in 1d198fbd68 in 2014 from issue #2232037: taxonomy_index should have a primary key.

For MySQL 8 it needs to be transaction_isolation instead of tx_isolation. Works fine for me.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -461,6 +461,11 @@ public static function open(array &$connection_options = []) {
+      // Set a low transaction level because otherwise busy Drupal sites run
+      // into transaction deadlocks.
+      // @todo Support more fine grained transaction isolation levels in
+      // https://www.drupal.org/project/drupal/issues/2572283
+      'isolation' => "SET SESSION tx_isolation='READ-COMMITTED'",

Should we code this directly in the connection calls if it will break some binary log modes? I don't know the answer but I worry there might be some unhappy people that apply an update and suddenly their site is completely broken.

N/A

For path #67. If there is an older MySQL version then it is fine.

For the newer MySQL 8 version it should be

transaction_isolation

So check first in MySQL

mysql> show variables like 'tx_isolation';
Empty set (0.00 sec)

If above result is empty then use

show variables like 'transaction_isolation'

Accordingly changes needed. Thanks

/drupal/web/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php

    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);

    $transaction = 'transaction_isolation';
    if (version_compare($version_server, '8.0.11', '<')) {
      $sql_mode .= ',NO_AUTO_CREATE_USER';
      $transaction = 'tx_isolation';
    }
    $connection_options['init_commands'] += [
      'sql_mode' => "SET sql_mode = '$sql_mode'",
      // Set a low transaction level because otherwise busy Drupal sites run
      // into transaction deadlocks.
      // @todo Support more fine grained transaction isolation levels in
      // https://www.drupal.org/project/drupal/issues/2572283
      'isolation' => "SET SESSION ".$transaction."='READ-COMMITTED'",
    ];

I have this error with migrate process: "Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO {key_value_expire} (name, collection, value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array...

Can I try patch in #67 or in #81?

I'm using Drupal 8.8.

Thanks...

Patch against 9.1.x

Hmm. Don't why, but since I added "transaction-isolation = READ-COMMITTED" our MariaDB 10.4 Galera cluster starts crashing every other day. Stuff like this is suddenly appearing in the logs and is then repeating endlessly while the DB won't accept any new connections:

Mai 29 11:33:58 yak1 mysqld[2479]: Mutex at 0x55c2e3150200, Mutex DICT_SYS created dict0dict.cc:824, lock var 2
Mai 29 11:33:58 yak1 mysqld[2479]: 2020-05-29 11:33:58 0 [Note] InnoDB: A semaphore wait:
Mai 29 11:33:58 yak1 mysqld[2479]: --Thread 139726468044544 has waited at dict0dict.cc line 880 for 41.00 seconds the semaphore:
Mai 29 11:33:58 yak1 mysqld[2479]: Mutex at 0x55c2e3150200, Mutex DICT_SYS created dict0dict.cc:824, lock var 2
Mai 29 11:33:58 yak1 mysqld[2479]: 2020-05-29 11:33:58 0 [Note] InnoDB: A semaphore wait:
Mai 29 11:33:58 yak1 mysqld[2479]: --Thread 139726627870464 has waited at dict0dict.cc line 8

I never had such a problem with the default ("REPEATABLE-READ"). I've removed the READ-COMMITTED for now. Is this really safe to enable also for Galera clusters?

Why not handling the transaction properly instead ? If cache, key-value store or other bits are identified as often being the cause of transaction deadlock, the proper SQL way of doing this probably is to let them isolated in REPEATABLE READ or SERIALIZABLE, do a SELECT FOR UPDATE when updating (and prey for your RDBMS to properly do predictive locking when inserting), and retry the transactions when they deadlock or fail at COMMIT. It would be easy to implement for cache or key-value if they are in standalone transactions, it would be extremely difficult to implement when they happen in a larger transaction that does business stuf (entity save I guess would be the most common scenario). It might worth the shot to investigate this thought.

We also discussed using a separate database connection for cache/lock etc. in #2347867: Race conditions with lock/cache using non-database storage - add a non-transactional database connection.

Having read the last few comments, this shold be on needs work as there is deep discussion and heady questions following the latest patch.

While I do not want to play issue ping-pong there's nothing to work on here, a decision needs to be made..

Did we discuss whether Galera is in scope? This patch, after all, just provides a default. We always provided a sensible default for most of Drupal users and provided override/extension points as necessary. Which this patch also does. Is it worth dropping / holding up this patch for Galera users? Mind you: I am neither for nor against it. I am just asking the question.

I think checking the version number to determine whether to use tx_isolation or transaction_isolation might be brittle. A more reliable way to know which of the two variables is supported by a given MySQL-alike database is to query the available variables:

SHOW VARIABLES WHERE variable_name IN ("tx_isolation", "transaction_isolation");

Rerolled for D9.1.

I tried to apply patch #84 to Drupal 9.1.2 but got composer error:

Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2020-05-04/1650930-default-mysql-tra...

The reason why tried to apply is that my migrate says sometimes:
SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction

If I remember right, this patch used to work on some older drupal 9 or 8.

@jukka792, you have to use the diff from the merge request !109, from the GitLab repo. You can simply go to the merge request and download as .diff, and then change the extension with .patch.
For simplicity i attached the file here from #94, which I can confirm it applies on D 9.1.2.

It may apply to 9.2 but it doesn't apply to 9.2 because of #3185231: Use combination SQL modes instead of explicit modes so it does need a reroll.

I still don't like that this can break replicated sites out of the blue because of a log mode setting that could be hard to change. I guess a site can get around that by adding the following to settings.php but I'm not sure how for them to get from a white screen of death to finding this hack.

$databases['default']['default']['init_commands']['isolation'] = '';

Re-rolled for 9.2.x

Reroll of #67 for Drupal core 9.1.x version.

$version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
This returns the wrong version for Azure Managed MySQL instances and connections fail as `NO_AUTO_CREATE_USER` was removed in MySQL 8.

See "Azure Database for MySQL server" reports wrong database version.

$this->connection->query('SELECT VERSION()')->fetchColumn();
This used as the fix in the issue above to fix this.

Here's an updated version (mainly for 9.1.x). I also removed NO_AUTO_CREATE_USER as it was not in 9.1.x either. If that's required I can add it again.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -206,8 +206,22 @@ public static function open(array &$connection_options = []) {
+ if (version_compare($server_version, '5.7.20', '<')) {

For MySQL 8 it needs to be transaction_isolation instead of tx_isolation. Works fine for me.

MariaDB still uses tx_isolation. And its Version numbers are higher, 10.x and above.
So this needs to be respected here.

Rerolled the patch in #107, thanks!

Here is a reroll for 8.9 in case someone needs it.

We ran this patch since years on various sites with different Drupal versions. In our use-cases we have massive drush commands with transactions and disabled memory caches. Without this patch you might corrupt the data because you miss the updated cache entries from other processes or page requests.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
   +    static $server_version;
   +
   +    if (!$server_version) {
   +      $server_version = $pdo->query('SELECT VERSION()')->fetchColumn();
   +    }
   

   Having this code in the database open function is something I really do not like. Yes, we had that in the past and I was very happy that we could remove it.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
   +    if (stripos($server_version, 'Maria') || version_compare($server_version, '5.7.20', '<')) {
   +      // This was deprecated in MySQL 5.7.20 and removed in 8. But MariaDB still
   +      // uses tx_isolation.
   +      $txn_isolation = 'tx_isolation';
   +    }
   

   Do we really need to do 2 different things here? One thing for MySQL and one for MariaDB. Can we set them both?

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
   +    if (stripos($server_version, 'Maria') || version_compare($server_version, '5.7.20', '<')) {
   

   The minimum required version for MySQL is now 5.7.8 and we are here testing for 5.7.20. Maybe the minimum required version for MySQL should become 5.7.20 for D10. Then this check can be removed in D10.

4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
   +      'isolation' => "SET SESSION " . $txn_isolation . " = 'READ-COMMITTED'",
   

   I understand that for most sites out there, this change will be an improvement. I just worry about the sites for which this change will result into errors or very slow sites. Especially for the ones where there is very limited knowledge about Drupal. Hoping that this change will not result in a clusterfuck.

5. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
   @@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
   +      // This was deprecated in MySQL 5.7.20 and removed in 8. But MariaDB still
   +      // uses tx_isolation.
   

   Could we add some links to the MySQL and MariaDB documentation about this settings.

6. Changing the tag "needs tests" to "needs manual testing"
7. The IS needs to be updated and a CR is needed as we are changing the default transaction isolation from "REPEATABLE READ" to "READ COMMITTED".

Wow, more concerns then lines of code ;-)

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
+ if (stripos($server_version, 'Maria') || version_compare($server_version, '5.7.20', '<')) {

The minimum required version for MySQL is now 5.7.8 and we are here testing for 5.7.20. Maybe the minimum required version for MySQL should become 5.7.20 for D10. Then this check can be removed in D10.

I agree with this.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
+ static $server_version;
+
+ if (!$server_version) {
+ $server_version = $pdo->query('SELECT VERSION()')->fetchColumn();
+ }

Having this code in the database open function is something I really do not like. Yes, we had that in the past and I was very happy that we could remove it.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -187,8 +187,23 @@ public static function open(array &$connection_options = []) {
+ if (stripos($server_version, 'Maria') || version_compare($server_version, '5.7.20', '<')) {
+ // This was deprecated in MySQL 5.7.20 and removed in 8. But MariaDB still
+ // uses tx_isolation.
+ $txn_isolation = 'tx_isolation';
+ }

Do we really need to do 2 different things here? One thing for MySQL and one for MariaDB. Can we set them both?

This could only be achieved when we split the drivers. Or let the MariaDB driver extend the MySQL driver. I wonder if this would be a smart decision anyway as both systems started to diverge.

This could only be achieved when we split the drivers. Or let the MariaDB driver extend the MySQL driver. I wonder if this would be a smart decision anyway as both systems started to diverge.

This is something that should be done in a new major release.

This is something that should be done in a new major release.

I agree.

So my suggestion is this:

• Introduce a dedicated MariaDB driver that simply extends the MySQL driver (in the first step)
• Introduce the transaction level as (advanced) configuration option of both drivers
• Select READ COMMITTED as default for new installations
• Don't set READ COMMITTED for existing installations automatically. People could manually switch to it.
• Write a a Release Note about it and mention that it is recommended to switch from the MySQL driver to the MariaDB driver if the site runs on MariaDB

How do we get an approval on this approach from the maintainers. I don't won't spent any time on this when it takes 7 more years for an approval.

Curious why this limited to mysql only, pgsql could be affected too and sqlite can have https://sqlite.org/pragma.html#pragma_read_uncommitted

would be great to update issue summary

@mkalkbrenner: I have asked @catch on Slack if he could give this issue some guidance, as he is one of the core release managers. He can make the decision. When it is a very big decision, he will bring it to the core committer meeting. @catch is also a core committer. Drupal 10 is to be released in the summer of 2022. It is very likely to be fixed in that release. When we get the ok and do the work.

Question on the patch first:

+    $txn_isolation = 'transaction_isolation';
+
+    if (stripos($server_version, 'Maria') || version_compare($server_version, '5.7.20', '<')) {
+      // This was deprecated in MySQL 5.7.20 and removed in 8. But MariaDB still
+      // uses tx_isolation.
+      $txn_isolation = 'tx_isolation';
+    }

It seems to me that both MySQL and MariaDB support the syntax:

SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED.

https://mariadb.com/kb/en/set-transaction/

https://dev.mysql.com/doc/refman/8.0/en/set-transaction.html

Is there a reason we can't use that? Then the discussion about separate drivers and versions might be moot.

If that's not possible:

1. I'm not sure about creating a new driver for this issue, it's cleaner when you look at it, but for example if it does turn out there's a cross-platform way to set this, then we'd have an unnecessary driver sitting around. Much easier to remove an if statement than deprecate a database driver and get everyone to migrate back.

2. If we do need to create a new driver, then that could (and should, so that people can change that before doing a major upgrade) happen in a minor release. The only thing we'd have to restrict to a major release, is removing MariaDB support from the MySQL driver.

3. I don't think the variable name changing is a pressing reason to increase the MySQL minimum version. Obviously we could remove the check if we do though.

I have talked to @catch on Slack and he said: "I don't think it needs a committers meeting yet - if we do the $settings option, we can have a follow-up to discuss actually changing the default altogether. I think we should change the default for brand new sites, just not existing ones."

Lets do that.

Removing the tags: ''Drupal 10" and "needs release managers review".

fwiw for changing the default, I think it comes under the category of 'disruptive bugfix' - i.e. it would have to be either a minor or major release, and probably requires some warning. It's likely to fix issues on far more sites than it breaks, but it's possible it'll break someone's site somewhere.

One option might be:

1. Add the option in settings, defaulting to the current
2. On sites where the default isn't set, show a notice in the status report that the site is using the default, and it's going to change to READ COMMITTED in Drupal X.x
3. Once we change the default, show a notice in the status report that the site is using the default and that the default is READ COMMITTED.

But we should move this over to the follow-up once it's created.

here's a unified patch as an intermediate step.

Patch does not apply.

Using the patch in #127 the transaction isolation level is now configurable during the site installation or in settings.php:

$databases['default']['default'] = [
  'database' => 'db',
  'username' => 'db',
  'password' => 'db',
  'host' => 'localhost,
  'driver' => 'mysql',
  'port' => 3306,
  'prefix' => '',
  'isolation_level' => 'READ COMMITTED',
];

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
   @@ -175,6 +175,17 @@ public function getFormOptions(array $database) {
   +    $form['advanced_options']['isolation_level'] = [
   

   I think that for this form element the average sites owner shall needs some info about the options. Which should he/she select and what is the default and what are the differences. But not too much. Something sensible.

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php
   @@ -175,6 +175,17 @@ public function getFormOptions(array $database) {
   +        'READ UNCOMMITTED' => 'READ UNCOMMITTED',
   

   Why did you add this option. I am not saying that you are wrong just curious.
   

We are gong to need some tests to make sure that the patch does what we want it to do:
1. That when we have an existing site and the setting isolation_level is not set, that it is using the isolation level REPEATABLE READ (query the database);
2. When we install a new site the key isolation_level is set in settings.php and its value is READ COMMITTED;
3. When we install a new site that it is using the isolation level READ COMMITTED (query the database);
4. When we have an existing sites and the key isolation_level is set in settings.php, that it is using the isolation level that is the same as in settings.php. Test for every possibility (at least for READ COMMITTED and REPEATABLE READ) (query the database).

I don't think we should offer this as an option on the form at all - default to READ COMMITTED, those who need something else can also edit settings.php

Like this?

I think that is what @catch wants. For the tests, you could look at the ones in the directory core/tests/Drupal/FunctionalTests/Installer/.

frequently getting errors while creating node and edit.

(
    [:db_condition_placeholder_0] => 4xx-response
)
" at /var/www/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php line 829" while reading response header from upstream, client: 123.XX.79.XX, server: example.com, request: "POST /node/4045123/edit?destination=/admin/content HTTP/1.1", upstream: "fastcgi://unix:/var/run/php/php7.2-fpm.sock:", host: "13.XX.XX.123, referrer: "http://13.XX.XX.123/node/4045123/edit?destination=/admin/content"
2021/08/05 16:03:56 [error] 23273#23273: *9266876 FastCGI sent in stderr: "PHP message: Uncaught PHP Exception Drupal\Core\Entity\EntityStorageException: "SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction: UPDATE {cachetags} SET invalidations=invalidations + 1
WHERE tag = :db_condition_placeholder_0; Array
(
    [:db_condition_placeholder_0] => node_list
)
" at /var/www/example.com/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php line 829" while reading response header from upstream, client: 123.XX.79.XX, server: example.com, request: "POST /admin/structure/entityqueue/asia_news/asia_news HTTP/1.1", upstream: "fastcgi://unix:/var/run/php/php7.2-fpm.sock:", host: "", referrer: "http://13.XX.XX.123/admin/structure/entityqueue/asia_news/asia_news"

On my D8 site, I had the same issue with deadlock and I added 'isolation_level' => 'READ COMMITTED', to my settings.php and it worked!

We use composer patches to apply patches. Merge request patch urls are updated for every commit on the branch, which makes it a moving target. Attached is a patch containing the current state of the merge request (https://git.drupalcode.org/project/drupal/-/merge_requests/109.patch).

#122 didn't work and #124 is empty
Is there a patch to 9.2.5 ?

I am getting this error with D9.2.6 and patch #109
Drupal\Core\Entity\EntityStorageException: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction: INSERT INTO "cache_entity"

I am deleting media content with multiple browser tabs (bulk delete)

Re-rolled patch. Please review.

still no settings.php control.

#131 adds the possibility to set the isolation_level in settings.php.

I tried to add test coverage that checks if the isolation_level is correctly added to the database settings when a new Drupal site is installed and when a site is installed with existing database settings.

Let's see what the testbot thinks.

I rerolled the patch for 9.4.x.

Looks like a few more tests need updating to deal with the new form element.

I started on updating the issue summary.

1. +++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
   @@ -219,10 +219,13 @@ public static function open(array &$connection_options = []) {
   +        // @todo Switch to 'READ COMMITTED' for Drupal 10, see https://www.drupal.org/node/1650930
   

   This comment is more than 80 lines.

2. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -175,6 +175,12 @@ public function getFormOptions(array $database) {
   +    // Add the isolation_level option to settings.php.
   +    $form['isolation_level'] = [
   +      '#type' => 'value',
   +      '#default_value' => empty($database['isolation_level']) ? 'READ COMMITTED' : $database['isolation_level'],
   +    ];
   

   Given it is just a value now we need to update the screenshot in the IS.
   Also, do we need to add this option to default.settings.php as per the comment here?
   I think we should add a change notice about this change.

1. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -175,6 +175,12 @@ public function getFormOptions(array $database) {
   +      '#default_value' => empty($database['isolation_level']) ? 'READ COMMITTED' : $database['isolation_level'],
   

   Should we not add more options here? There are 4 optuions according to MySQL. See: https://dev.mysql.com/doc/refman/8.0/en/innodb-transaction-isolation-lev...
   Now new sites are forced to use "READ COMMITTED" and existing sites are defaulted to "REPEATABLE READ". I would like to give sitebuilders more options.

2. +++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
   @@ -219,10 +219,13 @@ public static function open(array &$connection_options = []) {
   +        // @todo Switch to 'READ COMMITTED' for Drupal 10, see https://www.drupal.org/node/1650930
   +        'isolation_level' => 'REPEATABLE READ',
   

   To much indentation (2 spaces).

3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   @@ -0,0 +1,49 @@
   +    $connection_info['default']['isolation_level'] = 'REPEATABLE READ';
   

   Existing sites do not have this line in their setttings.php, therefor needs this line be removed.

4. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   @@ -0,0 +1,49 @@
   +    $this->assertStringContainsString("'isolation_level' => 'REPEATABLE READ',", $contents);
   

   This line should test that the "isolation_level" is not set in the settings.php.

   What needs to be added is to check the transaction isolation from the database. It should be "REPEATABLE READ". The query should be: SELECT @@SESSION.transaction_isolation. See: https://dev.mysql.com/doc/refman/5.7/en/set-transaction.html

5. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   --- /dev/null
   +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   
   +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   --- /dev/null
   +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerIsolationLevelNoDatabaseSettingsTest.php
   

   Both tests are MySQL only and therefor need to be moved to the mysql module.

We have now tests and therefor we no longer need manual testing.

Updated the IS and created the CR.

Hey! I tried to address both #150 and #151 here, so regarding #150:

1. Fixed! I tried to follow the current pattern that we have for @todos in the code, please check: LibraryDiscoveryParser.php#L166 for more details(3 spaces when we break the line).
2. We still need to update the screenshot on the IS, but I added a new comment at sites/default/default.settings.php documenting the new setting isolation_level

I think we should add a change notice about this change.

Thanks to @daffie, we have now a CR!

Moving to #151:

1. I think it would be nice to add this option on the form too, but following what @catch said on #130, the READ COMMITTED option will be the default and now the users can change this on settings.php, to help the users I followed the suggestion on #150.2 and documented this new option at default.settings.php, we just need to update the IS removing the screenshot.
2. I think it's fixed, please check what I said about it on #150.1
3. Fixed!
4. Fixed! I changed the assert to validate if the isolation_level is not set in the settings.php. I also added a test to validate the transaction_isolation, thanks for the query!
5. Fixed! I moved the tests into the MySQL module

Thanks!

@murilohp: Thank you for working on this.

1. Every change you make to sites/default/default.settings.php you also need to make to core/assets/scaffold/files/default.settings.php

2. +++ b/core/modules/mysql/src/Driver/Database/mysql/Install/Tasks.php
   @@ -175,6 +175,12 @@ public function getFormOptions(array $database) {
   +    $form['isolation_level'] = [
   +      '#type' => 'value',
   +      '#default_value' => empty($database['isolation_level']) ? 'READ COMMITTED' : $database['isolation_level'],
   +    ];
   

   This form needs to have all 4 possible transaction isolation level options in it. We should also add a text with something like: "The recommended option is 'READ COMMITTED'. Existing sites without this setting will default to 'REPEATABLE READ'. THe other 2 options 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-lev...."

3. +++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
   @@ -219,10 +219,14 @@ public static function open(array &$connection_options = []) {
        $connection_options['init_commands'] += [
          'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'",
   +      'isolation' => 'SET SESSION TRANSACTION ISOLATION LEVEL ' . $connection_options['isolation_level'],
        ];
   

   I think we should do a test to see if the set isolation level is one of the 4 possible options. If not, we should throw an new DatabaseTransactionIsolationLevelException. Just before the selected code.

4. +++ b/core/modules/mysql/tests/src/Functional/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   @@ -0,0 +1,55 @@
   +    $this->assertStringNotContainsString("'isolation_level' => 'REPEATABLE READ',", $contents);
   

   Checking for isolation_level is enough. We are testing that the isolation_level key is not set.

5. +++ b/core/modules/mysql/tests/src/Functional/InstallerIsolationLevelExistingDatabaseSettingsTest.php
   @@ -0,0 +1,55 @@
   +    unset($connection_info['default']['pdo'], $connection_info['default']['init_commands']);
   ...
   +    $this->settings['databases']['default'] = (object) [
   

   The part , $connection_info['default']['init_commands'] can be removed.

6. +++ b/sites/default/default.settings.php
   @@ -138,6 +138,16 @@
   + * For MySQL users only, you can change the transaction isolation level using
   + * 'isolation_level' setting. By default, MySQL will use 'READ COMMITTED'.
   

   Can we change this text to: "For MySQL, MariaDB or equivalent databases can the option 'isolation_level' be set. The recommended option is 'READ COMMITTED'. Existing sites without this setting will default to 'REPEATABLE READ'. 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-lev...."

7. +++ b/sites/default/default.settings.php
   @@ -138,6 +138,16 @@
   + * $databases['default']['default']['isolation_level'] = 'REPEATABLE READ';
   + * $databases['default']['default']['isolation_level'] = 'READ UNCOMMITTED';
   + * $databases['default']['default']['isolation_level'] = 'SERIALIZABLE';
   

   We are missing the option "READ COMMITTED".

8. +++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
   @@ -219,10 +219,14 @@ public static function open(array &$connection_options = []) {
   +        // @todo Switch to 'READ COMMITTED' for Drupal 10
   +        //   see https://www.drupal.org/node/1650930.
   +        'isolation_level' => 'REPEATABLE READ',
   

   Too much indentation. Please remove 2 spaces from the start of every line.

I'm not convinced it's useful to add this to the installer form, it seems like an invitation for people to shoot themselves in the foot. Just making sure we have decent documentation in default.settings.php seems enough here.

Hey I addressed #157 on this new patch.

1. Fixed! And thanks for the tip!
2. As @catch said on #158, we just need to make a decent documentation in default.settings.php
3. I think it's a great idea, I created DatabaseTransactionIsolationLevelException for this and a new kernel test to validate this exception.
4. Now that we have 'isolation_level' string inside both sites/default/default.settings.php and core/assets/scaffold/files/default.settings.php as a comment, how do we handle this? I mean I think this test will fail here.
5. Fixed!
6. I updated the documentation but the link exceeds 80 characters, is that a problem?
7. Added the option READ COMMITTED
8. Fixed!

Leaving as NW to see the testbot here. Also, it would be nice to have your thoughts here @daffie.

I am going to work on the tests.