Shopify / shipit-engine

Deployment coordination
https://shopify.engineering/introducing-shipit
MIT License
1.42k stars 145 forks source link

Fix stack deletion take two #1377

Closed DazWorrall closed 1 week ago

DazWorrall commented 1 week ago

Follow on: #1376

^ That PR didn't quite work, so I went a little deeper. Before that PR we ran a query that looked something like:

delete from `statuses` where `statuses`.`commit_id` in (1, 2, 3, 4, <all of them>)

... this is an indexed lookup:

id | select_type | table    | partitions | type  | possible_keys               | key                         | key_len | ref   | rows | filtered | Extra
--------------------------------------------------------------------------------------------------------------------------------------------------------------
1  | DELETE      | statuses |            | range | index_statuses_on_commit_id | index_statuses_on_commit_id | 5       | const | 4    | 100.0    | Using where

... however given the sheer amount of rows involved, the query times out or hits lock contention.

My PR changed the query to something like

delete from `statuses` where `statuses`.`commit_id` in (...) and `statuses`.`id` in (...)

...but that's a table scan:

id | select_type | table    | partitions | type | possible_keys                       | key | key_len | ref | rows | filtered | Extra
-------------------------------------------------------------------------------------------------------------------------------------------
1  | DELETE      | statuses |            | ALL  | PRIMARY,index_statuses_on_commit_id |     |         |     | 1    | 100.0    | Using where

Which also fails 🙃

Take two is to split the select and delete into separate, indexed queries, ran in batches. I also pre-emptively do this for Shipit::Commit as it uses the same pattern so could be affected in the same way.

casperisfine commented 1 week ago

I think we should just do Model.where(stack_id: xxx).limit(1000).delete_all in a loop until delete all return something < 1000.

This way you don't need the select.

casperisfine commented 1 week ago

(not 100% sure all 3 DBs support DELETE/LIMIT though)