Our current strategy for saving orders results in deadlock errors when many orders are saved concurrently.

The easiest way to reproduce is to execute the following Drush scripts in parallel (it should fail starting with 3 concurrent process):

<?php
for ($i = 0; $i < 500; $i++) {
 
$order = commerce_order_new();
 
$order_wrapper = entity_metadata_wrapper('commerce_order', $order);
 
$products = array(
   
1 => 30,
   
2 => 20,
  );
 
// Save the line items.
 
foreach ($products as $product_id => $quantity) {
    if (
$product = commerce_product_load($product_id)) {
     
$line_item = commerce_product_line_item_new($product, $quantity);
     
commerce_line_item_save($line_item);
     
$order_wrapper->commerce_line_items[] = $line_item;
    }
  }
 
commerce_order_save($order);
}
?>

The problem lies in that we have two unique keys on the order table: {commerce_order}.revision_id and {commerce_order}.order_number. When creating a new order, we insert '' (the empty string) into both columns, before changing it back in a subsequent UPDATE query. This process forces MySQL to acquire a lock on the index. It fails for some reason, and we end up with deadlock issues.

I suggest we insert NULL instead of the empty string, so that MySQL doesn't have to lock the unique key (NULLs are not comparable to anything, so they don't participate to UNIQUE constraints).

Files: 
CommentFileSizeAuthor
#7 1363826-deadlock-saving-order-6.patch3.16 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 3,546 pass(es).
[ View ]
#5 1363826-deadlock-saving-order-5.patch3.88 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 3,544 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
#2 1363826-deadlock-saving-order.patch2.79 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 3,511 pass(es).
[ View ]

Comments

Issue tags:+Performance

Tagging.

Status:Active» Needs review
StatusFileSize
new2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 3,511 pass(es).
[ View ]

Status:Needs review» Needs work

We need an update function for the schema changes.

Something like this one:

<?php
/**
* Allow null values for order_number and revision_id on {commerce_order}, and
* order_number on {commerce_order_revision}.
*/
function commerce_order_update_7107() {
 
$order_number_spec = array(
   
'description' => 'The order number displayed to the customer.',
   
'type' => 'varchar',
   
'length' => 255,
   
'not null' => FALSE,
  );
 
$revision_id_spec = array(
   
'description' => 'The current {commerce_order_revision}.revision_id version identifier.',
   
'type' => 'int',
   
'unsigned' => TRUE,
   
'not null' => FALSE,
   
'default' => 0,
  );
 
$order_number_revision_spec = array(
   
'description' => 'The order number displayed to the customer for this revision.',
   
'type' => 'varchar',
   
'length' => 255,
   
'not null' => FALSE,
  );
 
db_change_field('commerce_order', 'order_number', 'order_number', $order_number_spec);
 
db_change_field('commerce_order', 'revision_id', 'revision_id', $revision_id_spec);
 
db_change_field('commerce_order_revision', 'order_number', 'order_number', $order_number_revision_spec);
}
?>

My lack of git access is a sad story that will not be repeated here.

Note that the original issue for this (I think) got moved into Drupal core. #1320062: SQLSTATE[HY000]: General error: 1205 Lock wait timeout exceeded

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.88 KB
FAILED: [[SimpleTest]]: [MySQL] 3,544 pass(es), 0 fail(s), and 4 exception(es).
[ View ]

Confirmed deadlock errors using the script from the OP, and that the patch from #2 fixes them (and also shelves ~1s from the script running time).

Added the update function. Looks RTBC to me.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1363826-deadlock-saving-order-5.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.16 KB
PASSED: [[SimpleTest]]: [MySQL] 3,546 pass(es).
[ View ]

+++ b/modules/order/includes/commerce_order.controller.inc
@@ -119,6 +119,18 @@ class CommerceOrderEntityController extends DrupalCommerceEntityController {
+    // Pre-load all the line items of the orders in one go.
+    $line_item_ids = array();
+    foreach ($queried_orders as $order_id => $order) {
+      foreach ($order->commerce_line_items as $langcode => $items) {
+        foreach ($items as $delta => $item) {
+          $line_item_ids[$item['line_item_id']] = TRUE;
+        }
+      }
+    }
+    if ($line_item_ids) {
+      entity_load('commerce_line_item', array_keys($line_item_ids));
+    }
+  }

This is the part that's failing the tests. Maybe because the orders created in the CURD test have no line items attached to them?

Anyway, removed it from this patch as I think it deservers a new issue.

Status:Needs review» Reviewed & tested by the community

Let's do this. It has been sufficiently battle-tested in production.

Sorry for the pre-loading, it was definitely not part of this patch.

Related patch on the node table: #1369332: Avoid deadlock issues on the {node} table

Status:Reviewed & tested by the community» Fixed

Alrighty, finally reread this a couple of times to make sure I understood the issue. Seems like a great fix, and just a reminder, the reason order_number is inserted NULL is because we wanted to allow for token based order numbers that were dependent on values of the order object that aren't set until save. The core patch looks set to go in for node, too.

Still... I'm interested in the story behind bojanz's lack of git access. ; )

Also, you guys rock. Committed!

subscribing

Automatically closed -- issue fixed for 2 weeks with no activity.