Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a condition which would lead to MySQL deadlock #127

Merged
merged 1 commit into from
Jun 30, 2015

Conversation

gmacdougall
Copy link
Member

Running a delete by an index condition, for example:

DELETE FROM spree_adjustments
  WHERE adjustable_id = 1
  AND adjustable_type = 'Spree::Shipment'

will require MySQL to acquire a gap lock on the index which indexes
adjustable_id and adjustable_type.

Running this in a heavily loaded environment can lead to deadlocks in
MySQL as multiple threads need an exclusive gap lock on the index in
question.

By deleting records by the primary key instead, MySQL is able to deal
with this without requiring an exclusive lock on the primary key index,
avoiding the deadlock situation.

@athal7
Copy link
Contributor

athal7 commented Jun 30, 2015

👍

2 similar comments
@cbrunsdon
Copy link
Contributor

👍

@Senjai
Copy link
Contributor

Senjai commented Jun 30, 2015

👍

@jhawthorn
Copy link
Contributor

I don't think the where clause is necessary, I think the fix here is just using destroy_all, which will issue one DELETE for each record.

Old:
adjustments.shipping.delete_all

DELETE FROM spree_adjustments
  WHERE adjustable_id = 1
  AND adjustable_type = 'Spree::Shipment'

This PR:

Spree::Adjustment.where(id: adjustments.shipping.pluck(:id)).destroy_all

SELECT * FROM spree_adjustments
  WHERE adjustable_id = 1
  AND adjustable_type = 'Spree::Shipment';
SELECT * spree_adjustments
  WHERE id IN (1,2);
DELETE FROM spree_adjustments WHERE id = 1;
(callback queries)
DELETE FROM spree_adjustments WHERE id = 2;
(callback queries)

Proposed:

adjustments.shipping.destroy_all

SELECT * FROM spree_adjustments
  WHERE adjustable_id = 1
  AND adjustable_type = 'Spree::Shipment';
DELETE FROM spree_adjustments WHERE id = 1;
(callback queries)
DELETE FROM spree_adjustments WHERE id = 2;
(callback queries)

@gmacdougall
Copy link
Member Author

Thanks for that catch John. I think you're correct. I will make that modification.

Running a delete by an index condition, for example:

```
DELETE FROM spree_adjustments
  WHERE adjustable_id = 1
  AND adjustable_type = 'Spree::Shipment'
```

will require MySQL to acquire a gap lock on the index which indexes
adjustable_id and adjustable_type.

Running this in a heavily loaded environment can lead to deadlocks in
MySQL as multiple threads need an exclusive gap lock on the index in
question.

By deleting records by the primary key instead, MySQL is able to deal
with this without requiring an exclusive lock on the primary key index,
avoiding the deadlock situation.
jhawthorn added a commit that referenced this pull request Jun 30, 2015
Fix a condition which would lead to MySQL deadlock
@jhawthorn jhawthorn merged commit 99c5ccb into solidusio:master Jun 30, 2015
glongman added a commit to Fullscript/spree that referenced this pull request Apr 24, 2018
Tax computation occurs repeatedly per order, per line item, per step during checkout.
Spree::TaxRate computes new Adjustments after deleting the old ones.

A PR in Solidus pointed out that using `delete_all` can result in generated SQL that
contains polymorphic columns and caused INNODB to take gap locks. Under load this can result
in deadlock.

Using `deleted_all` means only primary key based deleted will occur. No gap locks so deadlocks can be
less likely.

spree_adjustments is a high traffic table. Spree::Adjustment has no `after_destroy` or relations that would invoke logic on destroy so this should be safe.

Ref: solidusio/solidus#127
spaghetticode referenced this pull request in nebulab/solidus Mar 26, 2021
Try to harden the PayPal integration spec
mamhoff added a commit to mamhoff/solidus that referenced this pull request Jun 26, 2024
…-callback-override

Remove `require_promotion_code` override
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants