Repository navigation
fix(insert): make the batch insert function give way instead of deadlocking (0.6) - #700
Merged
Merged
Conversation
…ocking `awa.insert_jobs_compat_batch` takes all of a batch's unique claims before any lane lock, while per-row inserts (generic `insert_many`, single-job inserts, released binaries) take each job's claim and its lane together. A per-row transaction holding a lane the batch needed, then inserting a key the batch had claimed, waited on the batch while the batch waited on it, and PostgreSQL aborted one with 40P01. The same happened with a transaction that enqueued lanes in separate statements in the opposite order to a batch. The function now takes its claims and lanes inside a subtransaction (an attempt). After the attempt's first lock, every lock wait ends within half of `deadlock_timeout` of it: claims are first inserted in one statement with a 1 ms lock_timeout, then key by key with the remaining budget, and each lane's registration and reservation get the remaining budget. A wait that runs out rolls the attempt back, releasing every claim and lane it took, and the batch retries after a short randomised pause. A per-row producer waiting on the batch is therefore released before its own deadlock check fires, and the batch never waits long enough to run one, so neither is the victim of a two-party cycle. Batches still order claims by key and lanes by lane, so they do not make each other retry. A lane whose wait ran out is awaited first on the next attempt, while the attempt holds nothing and with no bound beyond the caller's, so a lane kept busy by long per-row transactions cannot starve the batch. The caller's lock_timeout, when set, bounds the whole call. The ready rows and segments are written after the attempt, under the lane locks it took, and the result rows are built before the lock window as before, so the lane lock is held no longer than in 0.6.15. The patch probe now matches the wait budget, so `awa migrate` replaces the 0.6.15 body; a fixture of that body keeps the probe honest. New tests reproduce both deadlocks deterministically (they fail with 40P01 on the 0.6.15 body), cover the busy-lane fallback and the caller's lock_timeout, and race batch and per-row producers on shared keys and lanes asserting no 40P01 and that every key lands exactly once.
Adds the Unreleased changelog entries, a 0.6.15 to 0.6.16 upgrade guide and its README row, and updates the enqueue deadlock section of troubleshooting: batches through the function now give way to per-row producers and to multi-statement enqueue transactions, and the 0.6.15 body is replaced by `awa migrate`.
…tch-claim-deadlock-0.6 # Conflicts: # CHANGELOG.md # docs/upgrade-0.6.15-to-0.6.16.md
hardbyte
added
the
full-ci
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8459e6db5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -- waits long enough to run one. The caller's lock_timeout, when set, | ||
| -- bounds the whole insert. | ||
| v_caller_lock_timeout := current_setting('lock_timeout'); | ||
| v_wait_budget := current_setting('deadlock_timeout')::interval / 2; |
There was a problem hiding this comment.
When producers use different session-level deadlock_timeout values—for example, a batch connection configured to 20s and a per-row connection using the 1s default—this gives the batch a 10s wait budget while it still holds claims or lanes. In the documented claim/lane cycle, the per-row session therefore runs deadlock detection after 1s, before the batch releases its locks, so one transaction can still fail with 40P01. The retry budget cannot safely be derived solely from the batch session's setting; it must be below the possible peer detector delay or avoid waiting while holding these locks.
Useful? React with 👍 / 👎.
The wait budget was half of the batch session's own `deadlock_timeout`, but a per-row producer runs its deadlock check on its own session's setting, which the batch cannot see. A batch session at 20 s therefore held claims and lanes for 10 s while a per-row producer at the 1 s default was aborted with 40P01. The budget is now 500 ms, or half of the batch session's `deadlock_timeout` when that is lower, so per-row sessions at PostgreSQL's default or above are covered. The probe marker follows the new budget expression. The lane-window test no longer relies on a long batch budget: a probe that lands after the first attempt's budget is repeated rather than judged. A new test runs the claim cycle with the batch session at a 20 s `deadlock_timeout` and the per-row producer at the default; it fails with 40P01 without the cap. The patch upgrade rehearsal now expects a released 0.6.15 database to report both `insert_jobs_compat_batch` and `prune_controls` as pending, and the candidate migrate to apply both.
…tch-claim-deadlock-0.6 # Conflicts: # CHANGELOG.md
Retrying a batch attempt releases only what the attempt took. When the caller's transaction already held a lock from an earlier statement and a per-row producer waited for it while holding one of the batch's keys, the batch retried forever: every wait stayed under `deadlock_timeout`, so the batch never ran a deadlock check, and the per-row producer's single check found the cycle only if it happened to land during one of the batch's waits. Under sustained contention, alternating lost lanes could also keep the batch retrying for many seconds. The batch now retries with bounded waits for four wait budgets (2 s by default), then makes one final attempt with the caller's lock_timeout and ordinary waits, as the 0.6.15 function did. Every call therefore ends in success, a lock timeout or `deadlock detected`, and contention that outlasts the retries is served in lock order. This replaces the remembered first lane, which held a lane across the claims of later attempts, reset the wait budget after an unbounded wait, and tracked only one lane. The result rows are built once before any lock, and skipped rows are filtered out on return, so their construction no longer uses up an attempt's wait budget on large batches. Each lane registration statement now gets its own remaining budget. New tests cover a batch inside a transaction that already holds a lane, which looped without end before and now ends with one side aborted by `deadlock detected`; two lanes held past the bounded phase; and a 20,000-row batch that still gives way to a per-row producer. The probe marker follows the new body.
The first attempt inserts its claims in one statement and gave way after a 1 ms lock wait. Waits other than a claim of the same key in flight, such as relation extension, could pass 1 ms under load, so about 0.2-0.3% of calls in the lane contention benchmark retried although no keys overlapped. The probe now waits up to 10 ms per lock, or the remaining wait budget if shorter, and the patch probe matches the new body.
A batch gives way only while inserting its unique claims takes less than the 500 ms wait budget; past that every bounded attempt runs out before it reaches its lanes, and the ordinary final attempt behaves as in 0.6.15. On a slow CI runner the 20,000-row test batch was past that point and was correctly aborted with 40P01. The test now uses 5,000 rows, times an equivalent claim insert first, and requires the batch to give way only when that takes under half the budget; otherwise it requires the cycle to end as in 0.6.15. The docs state the size limit.
…tch-claim-deadlock-0.6 # Conflicts: # CHANGELOG.md # awa-model/src/insert.rs # awa/tests/patch_upgrade_rehearsal_test.rs # docs/troubleshooting.md # docs/upgrade-0.6.15-to-0.6.16.md
The batch function measured the caller's lock_timeout from entry and compared it cumulatively, so its own work, retries and successive waits all counted against it. Once spent, every bounded wait dropped to 1 ms and any timeout raised 55P03, where 0.6.15, which applied lock_timeout to each wait, succeeded. Each bounded wait is now the shorter of the remaining budget and the caller's lock_timeout, only a wait that the caller's value ended raises 55P03, and the final attempt applies the caller's lock_timeout per wait as 0.6.15 did. New tests cover a caller lock_timeout shorter than the batch's own work before its lane wait, which fails with 55P03 on the previous body, and pin the documented limit that a per-row transaction holding a lane past the bounded phase still deadlocks with the batch. The docs now state that limit, that a cycle through the caller's earlier locks ends after about 3 to 4 s and usually aborts the caller's transaction, and that the budget applies per wait rather than per statement.
This was referenced Nov 9, 2026
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.
Summary
Since 0.6.15,
awa.insert_jobs_compat_batch(used byinsert_many_copyandinsert_many_skip_unique_conflicts) takes all of a batch's unique claims before its lane locks, while per-row inserts — genericinsert_many, single-job inserts and every 0.6.14-and-earlier binary — take each job's claim and lane together. A per-row transaction holding a lane the batch needs, inserting a key the batch has claimed but not committed, deadlocked with it (40P01). The per-row side can't change, so the batch now gives way where it can, and is never worse than the 0.6.15 function.Design
The batch takes its claims, lane registrations and lane reservations inside one subtransaction (an attempt).
deadlock_timeoutif lower, and never longer than the caller'slock_timeout. The cap exists because the per-row producer's deadlock check runs on its own session's setting, which the batch can't see. Claims are first inserted in one statement that gives way to any in-flight claim of the same key (each lock wait capped at 10 ms, or the budget if shorter), then key by key on later attempts. Each lane registration statement and each reservation gets the remaining budget. A wait that runs out rolls the attempt back, releasing every claim, lane lock and registration row it took, and the batch retries after a 1–100 ms jittered pause.lock_timeoutand ordinary waits, as the 0.6.15 function did. Every call therefore ends in success,55P03or40P01; it never retries forever, and contention that outlasts the retries is served in lock order.lock_timeout. It applies per wait, as in 0.6.15: only a wait that the caller'slock_timeoutended raises55P03. The batch's own work and its retries do not count against it.v_bounded_until), so 0.6.15 databases seeinsert_jobs_compat_batchas pending; the 0.6.15 body is kept as a test fixture. A released 0.6.15awa migratestill treats the new body as applied, so rolling binaries back keeps it.What it covers, and what it doesn't
Covered: the two-party cycle between a batch and a per-row producer (shared key, or lanes enqueued in separate statements in the opposite order), when the batch call is the first thing its transaction locks, the per-row session's
deadlock_timeoutis above 500 ms (the default is 1 s), the batch inserts its claims within the budget, and the per-row transaction closes the cycle within the bounded phase. The batch gives way within its budget and the per-row transaction proceeds before its deadlock check runs.Not covered, and documented as such:
40P01as in 0.6.15 (pinned by a test).40P01after the bounded phase: after about 3–4 s instead of 0.6.15's 1–2 s, and usually by aborting the batch's (caller's) transaction rather than the per-row one.ON CONFLICT DO NOTHING, so roughly 10,000–20,000 keys on a fast server and fewer on a slow or busy one. Larger batches behave as in 0.6.15. Cycles of three or more transactions can also fall back to 0.6.15 behaviour.deadlock_timeoutat or below 500 ms can still be aborted by its own earlier deadlock check. Adeadlock_timeoutbelow about 3 ms on the batch session leaves no margin.Rejected alternatives: rolling back only the current lane (earlier claims stay held, livelock); taking lanes before claims (per-row transactions wait both ways across rows, and it lengthens #690's lane hold); catching
40P01in the function (the per-row side is usually the victim); retrying without a final ordinary attempt (spins forever on a cycle through the caller's earlier locks); remembering lost lanes and pre-locking them (reset the budget, held lanes across claims, and starved batches with two busy lanes).main's v055 has the 0.6.15 body and gets this in the 0.6.16 forward-port.Testing
New tests in
insert_many_batch_test, all on the skip and COPY paths where relevant:deadlock_timeoutand the per-row side at the default, a multi-statement producer in opposite lane order, and an 8×12 mixed-producer stress test asserting zero40P01, exact row accounting and each key landing once. These fail on the 0.6.15 body (the stress test saw 17 deadlocks) and pass now; the long-deadlock_timeouttest fails without the 500 ms cap.40P01.lock_timeoutshorter than the batch's own work before its lane wait: fails with55P03on the previous body, succeeds now as on 0.6.15.40P01), two lanes held past the bounded phase, the caller'slock_timeouton a lane held throughout, and a large-batch cycle that must give way when an equivalent claim insert takes under half the budget (5,000 keys take 90–100 ms here) and must otherwise end as in 0.6.15.insert_many_schema_fallback_test,enqueue_lane_lock_order_test,copy_test,integration_testandmigrate_patch_cli_testpass on PG17, as doespatch_upgrade_rehearsal_testagainst released 0.6.15 and 0.6.9, now expectingclaim_batch_job_index,insert_jobs_compat_batchandprune_controlspending on 0.6.15.Lane-contention benchmark, 16 producers on one lane, A/B on fresh databases with asynchronous commit (three runs each):
Single-producer p50 is 12.0 ms vs 11.6 ms. Throughput is within run-to-run noise. The p99 tail is not confirmed as retries: counting them on a busy machine, 0.2–0.3% of calls retried because the claim probe, then capped at 1 ms, tripped on a wait other than a shared key (the benchmark's keys never overlap), and lane retries appeared only when lane waits passed 500 ms under machine load. Under that load the 0.6.15 body's p99 rose as well (86 → 171 ms vs 116 → 220 ms). The probe now waits up to 10 ms per lock to avoid those spurious retries; the benchmark has not been re-run since.