Repository navigation
Let PUT cards swap fronts and lock cards in id order - #101
Merged
Merged
Conversation
Make card_deck_id_front_uc DEFERRABLE INITIALLY IMMEDIATE so uniqueness is checked at the end of the statement and one PUT can swap two cards' fronts. Join the UPDATE to a locked CTE that selects the rows ORDER BY id FOR UPDATE, so concurrent PUTs on overlapping cards lock them in the same order instead of deadlocking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two bugs in
PUT /decks/{id}/cards/. Each has a test that failed on main first.Swapping fronts in one PUT
Swapping
frontbetween two cards of a deck failed with a duplicate-key error.card_deck_id_front_ucwas not deferrable, and Postgres checks a non-deferrable unique constraint after each row, so the first row's new value collided with the second row's old one. The constraint is nowDEFERRABLE INITIALLY IMMEDIATE, with a migration that drops and recreates it. Postgres then checks it at the end of the statement, and a real duplicate still fails.DuplicateKeyError, andtest_update_cards_to_a_duplicate_frontstill passes.ON CONFLICTarbiter. The onlyON CONFLICTin the code is ondecks.idempotency_key, so nothing is affected.Deadlock between concurrent PUTs
The UPDATE plan is a nested loop driven by
unnest, so rows were locked in the order the client sent them. Two PUTs that touch the same cards in different orders could each hold a row the other needs. Sorting the input would not be enough, because the planner can pick a different join order.The UPDATE now joins a CTE:
Every updated row has to match a
lockedrow. The CTE locks rows in id order before each one is updated (it runs lazily, so the locks aren't all taken up front), and that is enough to keep the lock order the same across concurrent requests. It is still one statement and one round trip.idsis bound once and used twice, so the SQL text stays the same for any number of cards.Tests
test_update_cards_swaps_fronts: a PUT that swaps two fronts returns 200 with the swapped values.tests/test_card_locking.pyuses a real engine, because the interleaving needs separate connections:[1, 3, 2]and waits on card 3. PUT B sends[2, 1]. The test pollspg_stat_activityon a fresh connection each time, because the view is snapshotted per transaction. Then it releases the lock.DeadlockDetectedError. It does the same with thelockedjoin removed from this branch. With the join, both PUTs return 200, and the test passed 3 runs out of 3.test_autocommit_writes.pymoved toconftest.pyascommitting_engine, and both tests use it.just testpasses 87 tests at 100% coverage,just test-migrationspasses, andjust lintis clean.