Deadlock on cache_config (DatabaseBackend::setMultiple())

Any idea how this happened?

What's your transaction isolation level set to?

See #1898204: Do not use InnoDB for the semaphore table, use Memory and #1650930: Use READ COMMITTED by default for MySQL transactions for lots of discussion.

I'm seeing slightly different version of this, a lot:

Drupal\Core\Database\IntegrityConstraintViolationException</em>: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry &#039;block_content.type.basic&#039; for key &#039;PRIMARY&#039;: INSERT INTO {cache_config} (cid, data, expire, created, serialized, tags, checksum)

I'm not sure if it is the same problem, but it looks related.

This has to be something in setMultiple(), which I think explains why this is happening only for configuration, but it really isn't a configuration problem, it's a cache issue.

> SELECT * FROM information_schema.session_variables
    -> WHERE variable_name = 'tx_isolation';
+---------------+----------------+
| VARIABLE_NAME | VARIABLE_VALUE |
+---------------+----------------+
| TX_ISOLATION  | READ-COMMITTED |
+---------------+----------------+

Version: MariaDB 5.5.41

Based on how often I'm seeing and how annoying it is, I think this is critical.

maybe the different error is because of READ COMMITTED? I've just had an error locally as well, and there it was a deadlock.

See #1650930: Use READ COMMITTED by default for MySQL transactions for lots of discussion on READ_COMMITTED.

Discussed this a bit with @DamZ.

Looks like the best option, short of locking the whole process of fetching config from the database and putting them into cache, which would be quite expensive, is to simply ignore those kind of exceptions.

We more or less do the same in the Merge query implementation, where we have the same scenario (check if it exists and if not, insert it.), except there we play save and do the update of that row, but I think we don't need that in this case, two concurrent cache writes likely have the same information to put into the cache, otherwise that would be.. strange.

So, we should simply check for those two specific errors (deadlock and IntegrityConstraintViolationException) and ignore them. Do we need a special exception for Deadlock, just like we have for integrity constraints, so that we can abstract them from the used backend?

One other idea I had was to put a write lock on the whole table but I don't think we can do that in a database agnostic way.

Let's presume for a minute that we live in a mysql-only world and can use REPLACE.

Note: INSERT ON DUPLICATE KEY UPDATE would also be an option, but I have no idea how to use that with set multiple.

Note2: As discussed with @catch and also mentioned by @Fabianx somewhere, we can also have a special implementation that we use when mysql is enabled, but that won't actually solve this problem for different backends. So we probably still want to do the part about eating the exceptions.

Haha, fastest test run ever.

Looks like something has been eating those exceptions on setMultiple() previously.

Here's the same as an optional subclass that is used if the database type is mysql.

I like this, any numbers on performance, yet?

Found two nitpicks, rolled quickly.

I can reproduce the deadlock quite easily, just run something like ab -n 100 -c 10 http://d8/ and then do a drush cr while it is runner.

I see a few of those deadlocks every time I run it.

With the patch, they're gone. Did run drush cr in a row 5 times, now errors.

What's interesting is that I can however reproduce the PHP Fatal error: Class '__TwigTemplate_61f82e0aac09f5d1126d7ded09d615cd03eb2a460783f2d84db6dec1de74ecfa' errors with the same process. They come with a few notices and errors in the watchdog, will update the twig issue.

Not sure what else we need to do here? Maybe we should check that the error is silently ignored in the default implementation as well, and we should possibly make sure to test both implementations in the generic cache backend for mysql. not sure if we have an easy way to run a test just with MySQL? Or are the implicit tests everywhere else good enough?

The bug fix is RTBC, but for the new MySQL backend, I think we need:

a) some profiling
b) maybe some tests?

Not sure ...

Also: Berdir is this ready enough for warranting RTBC from your POV? (e.g. is the patch finished)

I wouldn't know what else to do, so I think so, yes. Since I'm building manual params and so on, the REPLACE query building should be double-checked for correctness and security.

a) Pretty hard to reliably profile I guess, as it depends a lot on your mysql configuration. What I've done is compare drush cr runtimes, 3 runs each.

HEAD: 5.69s, 5.81s, 5.72s => ~5.74
Patch: 5.61s, 5.49s, 5.31s => ~5.47

That's ~5% faster... (with xdebug and uprofiler disabled)

Uhm, I didn't make those numbers up, even if it looks like that ;) Either way, please run the same yourself to get different numbers.

b) Yeah, as mentioned, the problem is I think running that test only when the database type is mysql. Note that by default, the new backend is used, so *all* existing tests except DatabaseBackendUnitTest, which hardcodes the class name, are using the mysql specific implementation. So we have tons of coverage ,just not explicit one. We could have the same logic in the unit test class, but then we're not testing the default implementation anymore at all on mysql testbots..

RTBC, I am fine with the implicit coverage, the queries are easy enough. We hopefully have postgres and the unit test to test the non-SQL part of the cache backend.

I am happy with the benchmarks and the fix of the deadlock. Around 5% is what I expected :).

1. +++ b/core/lib/Drupal/Core/Cache/MySqlDatabaseBackend.php
   @@ -0,0 +1,82 @@
   +  protected function doSet($cid, $data, $expire, $tags) {
   

   Why not call through to doSetMultiple()?

2. +++ b/core/lib/Drupal/Core/Cache/MySqlDatabaseBackend.php
   @@ -0,0 +1,82 @@
   +    $this->connection->query("REPLACE {" . $this->bin . "} (cid, created, expire, tags, checksum, data, serialized) VALUES (:cid, :created, :expire, :tags, :checksum, :data, :serialized)", array(
   

   Needs an escapeTable().

3. +++ b/core/lib/Drupal/Core/Cache/MySqlDatabaseBackend.php
   @@ -0,0 +1,82 @@
   +    $query = "REPLACE {" . $this->bin . "} (cid, created, expire, tags, checksum, data, serialized) VALUES " . implode(', ', $query_parts);
   

   Same here with escapeTable()

Placeholder generation looks OK. This is pretty ugly but so are deadlocks.

Berdir asked me to test the patch from comment #15 with a postgreSQL database. I have ran "drush cr" 6 times and had no problems and no watchdog-messages.

With the postgreSQL database with the patch applied:
- I ran ab -n 100 -c 10 http://www.drupal80.com/
- While apache bench was running I did a drush cr
- After apache bench finished I did drush ws
From the watchdog:

 ID   Date          Type      Severity  Message                                 
 273  10/Mar 16:21  php                 Drupal\Component\Plugin\Exception\Plugi 
                                        nNotFoundException: The "" entity type  
                                        does not exist. in                      
                                        Drupal\Core\Entity\EntityManager->getDe 
                                        finition() (line 257 of                 
                                        /var/www/drupal80.com/drupal            
 272  10/Mar 16:21  php                 Notice: Undefined index: entity_type in 
                                        Drupal\views\Plugin\views\row\EntityRow 
                                        ->init() (line 93 of                    
                                        /var/www/drupal80.com/drupal/core/modul 
                                        es/views/src/Plugin/views/row/EntityRow 
                                        .php).                                  
 271  10/Mar 16:21  php                 Drupal\Component\Plugin\Exception\Plugi 
                                        nNotFoundException: The "" entity type  
                                        does not exist. in                      
                                        Drupal\Core\Entity\EntityManager->getDe 
                                        finition() (line 257 of                 
                                        /var/www/drupal80.com/drupal            
 270  10/Mar 16:21  php                 Notice: Undefined index: entity_type in 
                                        Drupal\views\Plugin\views\row\EntityRow 
                                        ->init() (line 93 of                    
                                        /var/www/drupal80.com/drupal/core/modul 
                                        es/views/src/Plugin/views/row/EntityRow 
                                        .php).                                  
 269  10/Mar 16:21  php                 Drupal\Component\Plugin\Exception\Plugi 
                                        nNotFoundException: The "" entity type  
                                        does not exist. in                      
                                        Drupal\Core\Entity\EntityManager->getDe 
                                        finition() (line 257 of                 
                                        /var/www/drupal80.com/drupal            
 268  10/Mar 16:21  php                 Notice: Undefined index: entity_type in 
                                        Drupal\views\Plugin\views\row\EntityRow 
                                        ->init() (line 93 of                    
                                        /var/www/drupal80.com/drupal/core/modul 
                                        es/views/src/Plugin/views/row/EntityRow 
                                        .php).                                  
 267  10/Mar 16:21  php                 Drupal\Component\Plugin\Exception\Plugi 
                                        nNotFoundException: The "" entity type  
                                        does not exist. in                      
                                        Drupal\Core\Entity\EntityManager->getDe 
                                        finition() (line 257 of                 
                                        /var/www/drupal80.com/drupal            
 266  10/Mar 16:21  php                 Drupal\Component\Plugin\Exception\Plugi 
                                        nNotFoundException: The "" entity type  
                                        does not exist. in                      
                                        Drupal\Core\Entity\EntityManager->getDe 
                                        finition() (line 257 of                 
                                        /var/www/drupal80.com/drupal            
 265  10/Mar 16:21  php                 Notice: Undefined index: entity_type in 
                                        Drupal\views\Plugin\views\row\EntityRow 
                                        ->init() (line 93 of                    
                                        /var/www/drupal80.com/drupal/core/modul 
                                        es/views/src/Plugin/views/row/EntityRow 
                                        .php).                                  
 264  10/Mar 16:21  php                 Notice: Undefined index: entity_type in 
                                        Drupal\views\Plugin\views\row\EntityRow 
                                        ->init() (line 93 of                    
                                        /var/www/drupal80.com/drupal/core/modul 
                                        es/views/src/Plugin/views/row/EntityRow 
                                        .php).

With no patch:

 ID   Date          Type      Severity  Message
 135  10/Mar 16:18  php                 Drupal\Core\Database\IntegrityConstrain
                                        tViolationException: SQLSTATE[23505]:
                                        Unique violation: 7 ERROR:  duplicate
                                        key value violates unique constraint
                                        "cache_config____pkey"
                                        DETAIL:  Key
 134  10/Mar 16:18  php                 Drupal\Core\Database\IntegrityConstrain
                                        tViolationException: SQLSTATE[23505]:
                                        Unique violation: 7 ERROR:  duplicate
                                        key value violates unique constraint
                                        "cache_config____pkey"
                                        DETAIL:  Key
 133  10/Mar 16:18  php                 Drupal\Core\Database\IntegrityConstrain
                                        tViolationException: SQLSTATE[23505]:
                                        Unique violation: 7 ERROR:  duplicate
                                        key value violates unique constraint
                                        "cache_config____pkey"
                                        DETAIL:  Key
 132  10/Mar 16:18  php                 Drupal\Core\Database\IntegrityConstrain
                                        tViolationException: SQLSTATE[23505]:
                                        Unique violation: 7 ERROR:  duplicate
                                        key value violates unique constraint
                                        "cache_config____pkey"
                                        DETAIL:  Key

Thanks, that's interesting. So it looks like PostgreSQL gets the same exception as MySQL in READ COMMITTED. And it's an exception we can catch, that the patch already does.. except that it then apparently fails on something weird...

Addressing the review from @catch, untested.

I can't reproduce the warnings from #23 on MySQL with READ COMMITTED and the new implementation commented out. I did get some rare serialization exceptions then, but not with the new implementation, so I'd consider this issue fixed with this patch.

I suggest you open a new issue to see if you can improve something for those problems on PostgreSQL.

Anyone wants to put this back to RTBC? :)

I assume tests passing is test enough of the interdiff :).

Back to RTBC

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
@@ -48,7 +48,13 @@ function __construct(Connection $connection, CacheTagsChecksumInterface $checksu
+    // Use the optimized mysql implementation if possible.
+    if ($this->connection->databaseType() == 'mysql') {
+      return new MySqlDatabaseBackend($this->connection, $this->checksumProvider, $bin);
+    }
+    else {
+      return new DatabaseBackend($this->connection, $this->checksumProvider, $bin);
+    }

Is this the right way to do this? Couldn't make a new query class of Replace (or something like that) that has different implementations depending on the driver? That way this is more re-usable and is easy to implement for SQLite which also can do replace queries.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.php
@@ -48,7 +48,13 @@ function __construct(Connection $connection, CacheTagsChecksumInterface $checksu
+    // Use the optimized mysql implementation if possible.
+    if ($this->connection->databaseType() == 'mysql') {
+      return new MySqlDatabaseBackend($this->connection, $this->checksumProvider, $bin);

Also please add that under some mysql versions this could cause transaction commit because cache_* and cachetags tables are created on demand

Agreed with #28.

Was chatting in IRC and I really think we should use INSERT ON DUPLICATE KEY UPDATE instead of REPLACE. I've been using "INSERT ON DUPLICATE KEY UPDATE" in my MySQL only cache backend module https://www.drupal.org/project/apdqc.

Anyway REPLACE has more drawbacks than INSERT ON DUPLICATE KEY UPDATE.
http://stackoverflow.com/questions/9168928/what-are-practical-difference...

REPLACE internally performs a delete and then an insert. This can cause problems if you have a foreign key constraint pointing at that row. In this situation the REPLACE could fail or worse: if your foreign key is set to cascade delete, the REPLACE will cause rows from other tables to be deleted

Insert on Duplicate Key update is almost 32 times faster.

One reason to use REPLACE is if you're using a FEDERATED Storage Engine; something that shouldn't be done for the cache tables: http://dev.mysql.com/doc/refman/5.0/en/federated-limitations.html

FEDERATED accepts INSERT ... ON DUPLICATE KEY UPDATE statements, but if a duplicate-key violation occurs, the statement fails with an error.

Another reason to use REPLACE is if the database does not fit inside the innodb buffer pool; thus the reads will hit disk; also if you're using TokuDB instead of InnoDB, REPLACE is better.
http://www.tokutek.com/2010/07/why-insert-on-duplicate-key-update-may-be...

See related: #2429659-96: Race conditions in the twig template cache.

1. Introduce a SimpleMerge class to MySQL using IODKU -- it should only be used if there is only one unique index (which is typically the primary key)
2. Add a mysql.cache.backend.database service utilizing it

I was considering something very similar to #35, but using a new Upsert query class instead, which will be implemented for PostgreSQL and SQLite as well.

Discussed with @chx in IRC and we're working together on it :)

The code is http://cgit.drupalcode.org/sandbox-chx-1857558/log/?h=upsert here, the two basic tests, stolen from merge pass on mysql. My involvement ends here , amateescu is working on the postgresql and sqlite implementantions and I hope he will add more tests and in general carry this to a happy end.

And I am gonna make that critical as I don't think we should release with a known and reproducable race condition. And obviously core commiters can decide if it should stay critical.

(Background: There was some discussion on IRC between catch and alexpott already, if PHP fatal errors that are reproducable making the system unusable are critical, so its important for the core maintainers to discuss this.)

Reasoning:

- drush cr might theoretically never complete as you always get a deadlock before it completes, also that random users get fatal PHP errors is quite a big problem.

And it is likely many other things in core and contrib will get into the same situation in the future.

Also this is a 100% reproducable problem with clear steps to reproduce.

Also this is a 100% reproducable problem with clear steps to reproduce.

I'm hitting this one way more often than the twig one.
Also the probably of hitting this afaik scales with the size of the site.

Those are pretty good reasons to make this critical.

I've updated https://www.drupal.org/core/issue-priority to clarify why issues like this should be critical - previously any 'not common PHP error' was more or less major by default which doesn't reflect the severity of something like this. https://www.drupal.org/node/45111/revisions/view/8664850/8729606

Discussed with @catch, @xjm, @effulgentsia and @webchick, we all agreed that this is a critical issue to be resolved before Drupal 8's release as this is easily reproducible on real sites with traffic.

AFAICT this is blocked on #2542776: Add an Upsert class. Updating as such.

(Not sure why #2542776 is not critical.)

When the blocker gets in, this is what we need to do here.

#2542776: Add an Upsert class is in.

The patch from #2542776: Add an Upsert class is not pushed yet.

Let's be consistent and use upsert for set() as well.

1. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -178,8 +178,9 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
      protected function doSet($cid, $data, $expire, $tags) {
        $fields = array(
   -      'created' => round(microtime(TRUE), 3),
   +      'cid' => $this->normalizeCid($cid),
          'expire' => $expire,
   

   Isn't it even easier if we just call setMultiple() in set() and drop all this completely?

   We used to do it because set() was faster, but if we now use an equally fast implementation, that should result in quite a bit less duplicated code?

2. +++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
   @@ -234,34 +235,20 @@ public function setMultiple(array $items) {
   +      // Only pass the values since the order of $fields matches the order of
   +      // the insert fields. This is a performance optimization to avoid
   +      // unnecessary loops within the method.
   +      $query->values(array_values($fields));
   

   Assume that I repeat my comment here from the other issue about whether this is worth it and if we really are considering to do micro-optimizations like this, why not just built a numeric array in the first place? readability?

I assume with the simplified implementation, it is no longer possible to replace the cache tag increment in DatabaseCacheTagsChecksum::invalidateTags() with this? I guess that's not a big issue, since cache tag invalidations really shouldn't have a high concurrency...

Going to set to needs review for #52. Wrt to 52.2 I think readability is a good reason. And it is not really a micro-optimisation with large cache inserts. 52.1 looks sensible to me - however it does make me think we're missing cid normalisation in setMultiple :(

Yup, removing all the set() code and passing everything to setMultiple() is a very good point. As for #52.2, readability++ :)

So we still need the code that ensures the cache table exists.

Ah nice that the cache table existence check is in setMultiple now. This is good work.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 94673d2 and pushed to 8.0.x. Thanks!

#57 had test failures for SQLite & PostgreSQL. Is that a problem, or?

@Wim Leers, it's not a new problem, we still have random failures for them.

Nice fix!

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.php
@@ -169,39 +178,19 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = array
-  protected function doSet($cid, $data, $expire, $tags) {
...
+  public function doSetMultiple(array $items) {

Filed a tiny follow-up to #2549745: Make DatabaseBackend::doSetMultiple protected.

We're still experiencing this deadlock issue on cache_config table on a production server running Nginx & MariaDB, whenever we run a drush cr (while in maintenance mode) during a composer and drush-based release deployment procedure. We cannot reproduce the error naturally on other environments.

For others landing on this issue first based on a Google search for the error message you're seeing, I recommend checking out these other related issues:

• #2833539: SQLSTATE[40001]: Serialization failure: 1213 Deadlock - OPEN without a PATCH, but several comments talk about different solutions that solve the problem for them, mostly related to the Mysql transaction isolation level (tx_isolation) , which can either be configured in mysql config file, or in Drupal's settings.local.php
• #1650930: Use READ COMMITTED by default for MySQL transactions - OPEN with PATCH that sets the tx_isolation value to READ-COMMITTED. Ideally, you'd try first adding the change in settings.local.php as mentioned above, then once confirmed this solves the problem, proceed with applying the patch on this issue.
• #2733675: Warning when mysql is not set to READ-COMMITTED - for context, reinforcing that the DB should be configured with READ-COMMITTED
• #2236395: extended db_transaction / Database::Connection::startTransaction to include isolation level - OPEN with PATCH that provides a significant overhaul of the db connection logic to better support setting transaction levels per query, instead of adjusting the heavy handed default transaction level for all queries in #1650930.

Finally, Acquia has good documentation on how to debug deadlock errors, and particularly mentioning using the Cache Debug module to get insight on the root causes, as well as setting the Isolation level to READ-COMMITTED. They also recommend that the isolation level should be set at the query level, not for all queries.

It's possible to avoid deadlocks by changing the transaction isolation (tx_isolation) variable from the default value of REPEATABLE-READ to READ-COMMITTED.

You can change the tx_isolation setting in your connection settings because it is both a session variable and a global variable. You can change it for all queries, or for select queries that are suspect. It's preferable that you modify specific queries, rather than changing tx_isolation for all queries or changing the overall database setting. This makes it easier to track down custom settings if the website begins behaving strangely.