views_content_cache_update_set is vulnerable to deadlocks/slow lock releasing when a lot of content being inserted (innodb)

I can confirm this... having the same on 7.x-3.0-alpha2

ha ha, Nice! I may ahve to look into this and see if there is a way to fix it...

Yeah...we can probably do better here, and we can probably also leverage the D7 DB API while we're at it.

Replace the delete/insert with a merge query, which adds a transaction, but should mean that we can do an UPDATE query most of the time.

Bad patch file.

This "may" be a fix, and this comment may be a separate issue, but why doesn't this module use the Cache API?

This is using the cache API for the bits that are using a cache (we use it via views).
This error is in a bit of code that in D8 would be using the 'state' system, because it isn't a cache thing, it's a thing that we want to keep around permanently.
I think the patch in #7 is good, but just need to check that it actually keeps the functionality of the module the same.

Actually, add some more assertions to the tests, so that we make sure the merge query is doing what it should.

Thanks everyone, I've pushed the latest patch into 7.x-3.x

I'd like to re-open this. We have deployed this module on drupal.org and are having fairly large issues with deadlocks around this table. Particularly around project_issue updates, as every one requires a X RECORD LOCK on a single row in this table to update the timestamp. The db_merge here doesn't really solve the problem.

I'm trying to get you some debugging information. Currently its looking a lot like the separate indexes for c1/c2/c3 are causing issues, but that is obviously not at all conclusive.

re-opening

Additional information:

The column/index split is indeed contributing to the issue. Has converting this from a c1,c2,cN split to a single combined cache-id ever been considered? It seems like this could be a two column table, (varchar,timestamp). However, I obviously have just started reading the module and likely don't know the specifics.

fix typo

Adding a related issue in the infrastructure queue.

I have same issue as:

PDOException: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: UPDATE {views_content_cache} SET timestamp=:db_update_placeholder_0

I fixed this by adding merge query within try catch block

I believe this patch is working for me. I did have to modify the directories by hand in the patch to get it to apply.

Getting this error with Version: 7.x-3.0-alpha3+1-dev
PDOException : SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded; try restarting transaction: UPDATE {views_content_cache} SET timestamp=:db_update_placeholder_0 WHERE ( (c2 = :db_condition_placeholder_0) AND (c3 = :db_condition_placeholder_1) ); Array ( [:db_update_placeholder_0] => 1458846330 [:db_condition_placeholder_0] => video [:db_condition_placeholder_1] => node_changed ) in views_content_cache_update_set() (line 128 in /sites/all/modules/views_content_cache/views_content_cache.module).

Updating the patch from #17 so that it will apply cleanly. I wonder at this approach; won't a try/catch just trade database lockouts for failed updates, resulting in views being cached too long?

I'ld be curious to hear thoughts on @nnewton's question in #14 above? Seems like that would allow the use of the default cache back-end, no?

I've noticed that if you update many nodes in the same request a merge is executed for each of them, which presumably locks the table for longer than necessary, resulting in higher deadlock probability.

How about only updating a maximum of once per map per request.

A quick proof of concept:

  function views_content_cache_update_set($object, $object_type, $timestamp = NULL) {
+   static $maps = array();
+ 
+   if (is_null($object) && is_null($object_type)) {
+     return $maps;
+   }
+ 
    $update = FALSE;
    // Important to default to time() instead of REQUEST_TIME to avoid race
    // conditions.
    $timestamp = isset($timestamp) ? $timestamp : time();
    if ($cids = views_content_cache_id_generate($object, $object_type)) {
      foreach ($cids as $cid) {
        if (!empty($cid)) {
          $update = TRUE;
          $mapped = views_content_cache_id_record($cid);
- 
-         // Ensure that the timestamp is recorded in the table.
-         $merge_query = db_merge('views_content_cache')
-           ->fields(array('timestamp' => $timestamp))
-           ->key($mapped)
-           ->execute();
+         $map_id = implode('_', $mapped);
+         $maps[$map_id] = array(
+           'timestamp' => $timestamp,
+           'mapped' => $mapped,
+         );
        }
      }
    }
    return $update;
  }

+ /**
+  * Implements hook_exit().
+  */
+ function views_content_cache_exit() {
+   $maps = views_content_cache_update_set(NULL, NULL);
+ 
+   foreach ($maps as $map) {
+     // Ensure that the timestamp is recorded in the table.
+     $merge_query = db_merge('views_content_cache')
+       ->fields(array('timestamp' => $map['timestamp']))
+       ->key($map['mapped'])
+       ->execute();
+   }
+ }

#26 seems to be working, I don't get deadlocks anymore, I will report back once I've tested it further.

dchatry - were you able to further test the proof of concept in #26?

Yes, the deadlocks are gone and I don't see any obvious additional issue.

Patch from #26, kudos to @MustangGB.

Did a simple rework of the last patch

Also, updated tests