Repository navigation
fix(postgres): order a PutItem that loses a create race after the winner - #388
Conversation
Racing puts to a new item must all succeed unless a put's own condition fails against the item before it, and their ReturnValues ALL_OLD images must form one chain, as on Amazon DynamoDB. The tests cover hash and range tables, a condition, a GSI table, and a stream table. Assisted-by: pi claude-opus-5-5
A PutItem takes the transactional path when it has a condition, ReturnValues ALL_OLD, ReturnConsumedCapacity, an index, a vector index, or a stream. BatchWriteItem puts take the same path on the same tables, and with ReturnConsumedCapacity INDEXES. On that path a put inserts a missing item with ON CONFLICT DO NOTHING. When a concurrent writer created the item first, the put returned ConditionalCheckFailed, even with no condition. BatchWriteItem returned it too, which Amazon DynamoDB never does. The put now re-reads the winner's row, checks its condition against it, and overwrites it, as UpdateItem already does. ALL_OLD, the index updates, and the stream record get the winner as the old image. If the winner was deleted before the re-read, the put retries the insert, up to 5 inserts. Storage tests hold the competing create open against a live PostgreSQL, and CI runs them. Assisted-by: pi claude-opus-5-5
The GSI and LSI tests give each put its own index key and check that only the final item's entry stays in the index. The stream tests check every round in sequence order: each record's old image is the record before it. New tests race BatchWriteItem puts on the stream table, and PutItem calls with ReturnConsumedCapacity. Assisted-by: pi claude-opus-5-5
Two storage tests park the put's insert behind a trigger while an outside transaction commits a winner and a second one locks it. The winner is then deleted before the put re-reads it. In one test the put retries its insert and creates the item. In the other it loses five inserts and returns an internal error, with nothing written. The create race tests now wait for a backend that the creator blocks, not for any lock waiter. Assisted-by: pi claude-opus-5-5
The high-level design says that a missing item has no row to lock, and how a PutItem or UpdateItem that loses the insert orders itself after the winner. Two comments in the put code now match what the code does. Assisted-by: pi claude-opus-5-5
The deleted-winner and give-up tests now run on a hash table and on a hash and range table, so the retry arm and the bound of the range branch have tests too. When a driver step fails, the driver opens the gate, and the test reports what the put returned instead of only that no backend waited. Assisted-by: pi claude-opus-5-5
The module doc of the storage tests still said two. Assisted-by: pi claude-opus-5-5
On MongoDB, a put with no condition on a table with no index and no stream returns HTTP 500 when it loses the create race, and the MongoDB write-race fix repairs it. That fix lands as its own PR. Until it is on main, the three affected tests are marked xfail when the MongoDB test runner runs them. The marker is not strict, so the tests also pass once the fix is in. Remove the marker then. Assisted-by: pi claude-opus-5-5
robinnsc
left a comment
robinnsc
left a comment
There was a problem hiding this comment.
Walked through both the sort-key and hash-only branches; the lost-insert path re-reads FOR UPDATE, checks the condition against the winner, overwrites it, and the winner flows through as the old image for ALL_OLD, the stream record and index sync. The retry loop mirrors UpdateItem's. Postgres unit tests pass locally; I didn't have a live PG to run put_create_race. A few small notes inline.
| // returns no row and the insert is retried. | ||
| attempt += 1; | ||
| if attempt >= MAX_CREATE_RACE_ATTEMPTS { | ||
| return Err(create_race_exhausted(attempt)); |
There was a problem hiding this comment.
The cap is checked after the fifth lost insert and before the re-read, so if the fifth winner is still present the put gives up rather than overwriting it. That matches UpdateItem exactly and the new boundary test pins it deliberately, so I don't think the behaviour is wrong, but the message "a concurrent writer repeatedly created and deleted the row" is not quite true in that case, since the last winner stayed. Could consider either one final re-read before the error so the message holds, or rewording the message to something like "lost the create race 5 times" so it stays accurate for both cases. Same applies to the hash-only branch at line 334.
There was a problem hiding this comment.
Good point, the message wasn't true when the last winner stays. Reworded to "each insert lost the create race to a concurrent writer". I kept the give-up point the same as UpdateItem.
| return key | ||
|
|
||
|
|
||
| def _race( |
There was a problem hiding this comment.
The GSI, LSI and stream tests only synchronise the 8 writers at the barrier before they send, so whether any of them actually reaches the lost-insert branch depends on timing; a run where every put serialises through the existing-row path would still pass. The Rust tests do hit the branch deterministically, but they use a table with no indexes and no stream, so the "winner as old image for index and stream maintenance" claim is only covered by the mutant check in the description rather than by a test that must fail without it. One option would be a Rust test that holds the create open on a table with an LSI and a stream and checks the resulting rows and record, which would make that coverage independent of scheduling.
There was a problem hiding this comment.
Fair, the wire tests only hit that path when the timing allows. Added a storage test
| XFAIL_UNTIL_MONGODB_FIX = pytest.mark.xfail( | ||
| bool(os.environ.get("EXTENDDB_TEST_MONGODB_CONTAINER", "").strip()), | ||
| reason="MongoDB returns HTTP 500 for a lost create race, fixed by the MongoDB write-race fix", | ||
| strict=False, |
There was a problem hiding this comment.
Same as #387: the MongoDB xfail is whole-test and strict=False, so unrelated failures on MongoDB show as XFAIL and the eventual fix will XPASS without forcing the marker out. strict=True or a TODO naming the MongoDB PR would make it self-expiring.
There was a problem hiding this comment.
Added raises=AssertionError and a TODO. I left it non-strict on purpose: without the fix, one of these tests still passed in 3 of 30 runs on MongoDB, so strict would make CI flaky. The #387 marker is strict because those tests fail every time.
The error said that a concurrent writer repeatedly created and deleted the row. When the put gives up, its last winner can still be there, so the message now says that each insert lost the create race to a concurrent writer. Assisted-by: pi claude-opus-5-5
The wire tests reach the lost-insert path only when the timing allows it. This storage test holds a transactional create of the item open on a table with an LSI and a stream, so the put always loses the insert. It checks that only the put's LSI row is left and that the stream holds the INSERT and then a MODIFY whose old image is the winner. Dropping the old image for the index or for the stream fails it. Assisted-by: pi claude-opus-5-5
The MongoDB xfail marker now expects an AssertionError, so an unrelated error no longer shows up as an expected failure, and it names the fix it waits for. It stays non-strict: without the fix, one of these tests still passed in 3 of 30 runs on MongoDB, so a strict marker would make CI flaky. Assisted-by: pi claude-opus-5-5
2 participants
What
On PostgreSQL, a PutItem that lost the race to create a new item returned
ConditionalCheckFailedException, even when it had no condition. It now orders itself after the winner, as UpdateItem already does.put_item_impl()incrates/storage-postgres/src/data/put_item.rs: when theINSERT ... ON CONFLICT DO NOTHINGof a missing item affects no row, the put re-reads the rowFOR UPDATE, checks its condition against the winner, and overwrites it. ReturnValues ALL_OLD returns the winner, and the stream record and index maintenance use it as the old image. If the winner was deleted before the re-read, the insert is retried, up to 5 inserts, the same bound as UpdateItem.docs/design/02-high-level-design.md.SQLite runs one writer at a time, so it does not have this bug. On MongoDB the losing put gets HTTP 500 instead, and a separate PR fixes that.
Why
Found while checking TransactGetItems isolation: 8 clients put the same new key at once. Measured on
c178814:attribute_not_exists(zz), which every racer passes, hash tableattribute_not_exists(pk)(control)On Amazon DynamoDB the ALL_OLD images of one round form a single chain, from no item to the final one, so the puts were applied one after another.
Fixes: n/a, found by a concurrency probe, no issue filed
Result
test_put_item_create_race.py(9 tests) fails 8 of 9 on main. It passes 9 of 9 on this branch in every run, on SQLite, and on MongoDB with its fix. 7 of the 8 new PostgreSQL storage tests fail on main, and all 8 pass here. Two mutants, one that drops the old image for the index updates and one that drops it for the stream record, each fail the matching tests.cargo test --release --workspace: 1,230 passed.Testing done
tests/test_put_item_create_race.py(new, dual-target): 8 clients put the same new key, 15 rounds per test. Shapes: ReturnValues ALL_OLD on hash and hash-range tables, ReturnConsumedCapacity, a condition every racer passes, a GSI table, an LSI table, a stream table, BatchWriteItem on the stream table, and theattribute_not_exists(pk)control. Every put must succeed unless its own condition fails, and the ALL_OLD images must form one chain. The GSI and LSI must hold only the final item's entry. For each key, the stream must hold one INSERT and then MODIFY records in sequence order, each with the record before it as its old image.crates/storage-postgres/tests/put_create_race.rs(new, needsEXTENDDB_TEST_PG_CONNECTION_STRING): an outside transaction holds an uncommitted insert of the item, and the test commits it once the put waits on it. The put must overwrite the winner and return it as ALL_OLD on hash and hash-range tables, pass a condition that holds for the winner, and fail a condition that the winner breaks, returning the winner. Four more tests park the put's insert behind a trigger and delete the winner before the put re-reads it, on hash and hash-range tables: the put retries and creates the item, and after 5 lost inserts it gives up with nothing written.Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
ADR / RFC: n/a. One backend's create-race handling; no wire, trait, auth, on-disk, or CLI change.
Merge order: after the MongoDB fix for the same race, without which the new test gets HTTP 500s on MongoDB. The CI step conflicts by one line with #385: keep both
--testflags.Breaking changes
None.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.