Fix query which creates significant deadlock potential.

[colinmollenhour]

While doing some database performance testing with Magento (e.g. 16 processes simultaneously creating 100 orders each) I noticed a significant number of deadlocks occurring, especially with newer and more optimized versions of MySQL (5.7 & MariaDb 10.1). By inspecting the general query log and show engine innodb status I determined that the issue was that the SELECT si.*, p.type_id ... FOR UPDATE query used by registerProductsForSale() method was the culprit. When FOR UPDATE is used an X lock is taken on all matching rows and an S lock is taken on all rows linked by foreign keys. This applies also to joined tables, so joining catalog_product_entity in this FOR UPDATE is causing an X lock on catalog_product_entity which I don't see as being at all necessary, and it also causes an S lock to be taken on all rows linked to cpe by a foreign key (eav_attribute_set and eav_entity_type).

Here is an example deadlock:

------------------------
LATEST DETECTED DEADLOCK
------------------------
2016-09-28 20:59:17 7f46bb7e7700
*** (1) TRANSACTION:
TRANSACTION 453781, ACTIVE 11 sec starting index read
mysql tables in use 1, locked 1
LOCK WAIT 16 lock struct(s), heap size 2936, 12 row lock(s), undo log entries 13
MySQL thread id 273, OS thread handle 0x7f46bb754700, query id 2689415 10.0.1.5 root statistics
SELECT `eav_entity_store`.* FROM `eav_entity_store` WHERE (entity_type_id = '11') AND (store_id = '3') FOR UPDATE
*** (1) WAITING FOR THIS LOCK TO BE GRANTED:
RECORD LOCKS space id 255 page no 4 n bits 120 index `entity_type_id_store_id` of table `magento`.`eav_entity_store` trx
table locks 9 total table locks 6  trx id 453781 lock_mode X locks rec but not gap waiting lock hold time 4 wait time before grant 0
*** (2) TRANSACTION:
TRANSACTION 453786, ACTIVE 9 sec starting index read, thread declared inside InnoDB 4998
mysql tables in use 2, locked 2
115 lock struct(s), heap size 30248, 1767 row lock(s), undo log entries 2417
MySQL thread id 271, OS thread handle 0x7f46bb7e7700, query id 2693906 10.0.1.5 root Sending data
SELECT `si`.*, `p`.`type_id` FROM `cataloginventory_stock_item` AS `si`
 INNER JOIN `catalog_product_entity` AS `p` ON p.entity_id=si.product_id WHERE (stock_id=1) AND (product_id IN(1454, 274, 842, 464, 2
87, 826)) FOR UPDATE
*** (2) HOLDS THE LOCK(S):
RECORD LOCKS space id 255 page no 4 n bits 120 index `entity_type_id_store_id` of table `magento`.`eav_entity_store` trx
table locks 20 total table locks 6  trx id 453786 lock_mode X locks rec but not gap lock hold time 4 wait time before grant 0
*** (2) WAITING FOR THIS LOCK TO BE GRANTED:
RECORD LOCKS space id 84 page no 8 n bits 304 index `PRIMARY` of table `magento`.`catalog_product_entity` trx table locks
 20 total table locks 3  trx id 453786 lock_mode X locks rec but not gap waiting lock hold time 0 wait time before grant 0
*** WE ROLL BACK TRANSACTION (1)

Notice how txn 2 has 115 lock struct(s), heap size 30248, 1767 row lock(s), undo log entries 2417 even though it is only locking 6 stock items. By separating the SELECT ... JOIN ... FOR UPDATE into two separate selects with only the one on cataloginventory_stock_item using FOR UPDATE the lock still ensures correct inventory updates but will no longer create an X lock on cpe or a massive number of S locks on a bunch of other tables.

Note, there is also room for improvement in reducing lock contention on fetchNewIncrementId which I have fixed years ago but I will open a separate PR for that issue or perhaps add it to this one. In short, add unique key on eav_entity_store (entity_type_id, store_id) and drop foreign keys and indexes on entity_type_id and store_id. The unique key fixes a bug where there can be two conflicting rows and also allows innodb to lock only a single row and removing the foreign keys gets rid of unnecessary S locks on other tables.

What should the version numbering be for database upgrade scripts to avoid conflicts with future upstream upgrade scripts?

[alexkirsch]

Is there no workaround for this? Would a sub query work? Not in love with the separate SQL request.

[colinmollenhour]

@alexkirsch It will add like 1ms to the overall order creation time. I'll take that any day to avoid deadlocks on critical transactions... In many cases two queries is far faster than subqueries anyway.

[drobinson]

@alexkirsch Looks like the code wrapping this starts a transaction anyway, so separate sql queries doesn't seem any more dangerous from that aspect either.

Awesome research @colinmollenhour - we've always had trouble with deadlocks, and simply re-tried transactions in some cases in the past. Seems like this might remedy those situations once and for all.

About the fetchNewIncrementId deadlocks - I would say that definitely deserves its own PR if we'll need to talk about versioning core install scripts. It would be nice if Magento would admit that they're dropping support for 1.* some time soon.

[tmotyl]

for the versioning reasons, we could provide new module and put the update scripts in there. This way we can avoid conflicts.

[colinmollenhour]

@tmotyl The problem with using other modules is sometimes the order of operations matters. E.g. you can't change an index on a table that doesn't exist yet. I haven't tested it yet but from looking at the setup code I think it should be possible to add "patch" versions that would fit in between official versions.

Example:

1.6.2.1-1.6.2.2 (official)
1.6.2.2-1.6.2.2-lts1 (openmage)
1.6.2.2-1.6.2.3 (future official version)

[tmotyl]

if that works, then fine. the "new module" could depend on other modules, so we;ve got order covered.
In general big +1 for that, I was also hit by the deadlocks.

[colinmollenhour]

Using module dependencies would work for most cases but you might still run into issues. Using the patch versions would ensure that the update order is always the same so later updates from core could not break your patch updates (although the patch updates could break later core updates but you'd just have to deal with those when merging). Since both might require manual fixing of conflicts the difference between using depends vs patch versions is that with patch versions you know what was already applied for existing installations and can incrementally make updates from there whereas with depends if you later change the upgrade scripts to fix conflicts it is hard to write the update so that it works for both new and existing installations. I've run into this before where making modifications to core tables in an external module got ugly when I decided to update the core modules. In general the least problematic way is to apply the updates in the module that creates/defines the table you're updating.

[colinmollenhour]

So is anyone going to review and either reject or approve this? :)

[drobinson]

@Flyingmana @LeeSaferite Any opinion? Sounds like this could solve some problems @LeeSaferite has seen in the past...

[drobinson]

@tmotyl does this PR get +1 from you?

[tmotyl]

@drobinson +1 by reading only, need to find some time to test it.

[Flyingmana]

This change looks absolute reasonable after reading a bit into how "FOR UPDATE" works ( http://stackoverflow.com/a/27936027/716029 )

There may be a small chance for conflicts when productTypes change (does magento even have a feature for this which could go wrong?) but I dont think that would happen in a way which renders a stock change wrong.

So, +1 from me

[colinmollenhour]

Congrats OpenMage team, you beat the Magento 2 team to merging! :) In fact it appears the M2 PR hasn't even been looked at yet.. Oh well.

[paveq]

I think this does not do what was intended. Product type is not sent on $rows array.

Eg. this could work, or alternatively loop by reference.

foreach ($rows as $k => $row) { $rows[$k]['type_id'] = $typeIds[$row['product_id']]; }

[colinmollenhour]

You are right but it was fixed a few days later on 7804594.

[tmotyl]

@colinmollenhour did you managed to open the PR you mentioned:

Note, there is also room for improvement in reducing lock contention on fetchNewIncrementId
which I have fixed years ago but I will open a separate PR for that issue or perhaps add it to this one