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):

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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Issue tags: +Performance

Tagging.

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
2.79 KB
bojanz’s picture

Status: Needs review » Needs work

We need an update function for the schema changes.

Something like this one:

/**
 * 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.

rfay’s picture

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

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.88 KB

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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.16 KB
+++ 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.

Damien Tournoud’s picture

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.

Damien Tournoud’s picture

rszrama’s picture

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!

guy_schneerson’s picture

subscribing

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