Repository navigation
core/storage: keep a cursor's gap position across a peer cursor's balance - #10685
turso-github-handyman[bot] wants to merge 4 commits into
Conversation
Autofixer merge readiness: ✅ Recommended
TL;DR: In a transaction, a join's outer scan returned 50|1,50|2 again after DELETE of a leaf's last row then an INSERT that split the leaf. Head saves gap cursors as GE{eq_only:false}/GT. Review: try_save_position_for_external_balance gap branch. Least sure about: How the audit checked itChange: Invariant violation · Risk if wrong: medium · Behaviour: checked · Audited:
Commands the audit ran (8)
Autofixer's recommendation; a maintainer decides. Updates when the patch, CI, or main changes. |
Autofixer verifier historyVerifiers raised 4 blocker or major findings while Autofixer worked on this pull request. Later rounds may have fixed them; the merge audit re-checks each one. Findings, oldest first
|
Autofixer breaker follow-upsThe breaker found 3 defects that
|
|
Resumed by a maintainer's mention of Autofixer on 2026-10-25T03:49:41.273Z; Autofixer follows up on this pull request again. |
|
@turso-github-handyman continue this |
A restore that does not find its row leaves the cursor with no record and skip_advance set. The save before a peer's balance skipped such cursors, so after a leaf split the next Next walked a stale page stack and returned earlier rows again. Save the entry after the gap (GE) or the last cell of the leaf (GT), and put the cursor back in the gap when it is restored.
|
Finished the remaining work. No code change was needed: the btree unit tests and the integration test from the issue fail without the fix (the join returns 50|1 and 50|2 a second time) and pass with it. All 85 btree tests and the 12 whopper regression tests pass. Of the 486 query_processing integration tests, 485 pass; the one failure needs io_uring, which the sandbox blocks. fmt and clippy on turso_core are clean. Whopper (btree-rebalance seeds 1 and 2, fast seed 3) and limbo_sim (seed 7) pass, but a temporary log line showed none of these seeds runs the new branch, because the simulators never write a table while the same connection has a scan open on it. I amended the commit message to the repo style ( — Autofixer (34d67f5) |
a76a9b7 to
34d67f5
Compare
|
Autofixer corrected the pull request's text to address the merge audit's findings on 34d67f5. The code is unchanged, so no verifier or breaker ran; a fresh audit checks the pull request. |
…lance A peer cursor left on the row after a deleted mid-leaf row has a record and skip_advance set; the save before a balance stored it as an exact GE seek, so the restore dropped skip_advance and the next Next skipped that row. Save any cursor with skip_advance as sitting in the gap before its cell. Autofixer-Repair: audit
|
Autofixer pushed 5494530 to address the merge audit's findings. |
Merging this PR will improve performance by 23.51%3 warnings, 1 note
Performance Changes
Tip Curious why performance improved? Comment Comparing Footnotes
|
|
@turso-github-handyman is this change truly minimal? if it is, defend why. if it isn't, make it so. |
…ance The doc comment on stay_before_entry_if_saved_in_gap still said only cursors with no record are saved as a gap; since the previous commit any cursor with skip_advance is. Autofixer-Repair: review
|
Kept the fix as is; the code part is minimal. The bug is one function, try_save_position_for_external_balance, dropping skip_advance before another cursor of the same connection splits a leaf. It did this in two ways: it skipped cursors with no record (the issue's case, 50|1 and 50|2 returned again), and it saved cursors with a record by an exact seek, so the next Next skipped a row (deleting row 30 loses 40|1). Same flag, same lines; fixing one case would knowingly leave the other. Code change apart from tests: (1) the save rule: a cursor with skip_advance is saved as the gap before its cell, using that cell's key with GE{eq_only:false}, or the leaf's last cell with GT when it sits after it; (2) a three-line helper called where restore_context finishes, which puts the cursor back in the gap. Every other caller that saves a position uses GE{eq_only:true} (btree.rs:3293, 3340, 7082, 7090), so no other path changes; (3) for index cursors, moving onto the key cell to copy the record and back (the copy code moved into a helper unchanged), without which an index cursor in a gap panics on record.expect. Simpler versions I checked: re-seeking the last cell exactly would read row 50 instead of NULL for the deleted row, unlike SQLite; MustInvalidate loses the position; carrying the flag in CursorContext as SQLite does needs a second flag for no record, so it is more code. I removed parts one at a time: without the helper call in the RequireAdvance path, index_peer_after_last_cell_of_leaf_continues_forward_after_two_inserts fails; without cell_count > 0, cell_count - 1 underflows on an emptied tree. No test reaches the is_leaf check; it is there because a position past the last cell of an interior page is a child pointer, not a row, so there is no key to save, and like the cell_i — Autofixer (5e1860c) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Resumed by Autofixer once only maintainers had pushed to the branch besides it on Oct 25, 14:05 UTC; Autofixer follows up on this pull request again. |
|
Autofixer no longer pushes to this branch. A maintainer ( Autofixer still audits each new head and keeps the merge readiness and its labels current. To ask it something, mention To hand it back: remove the maintainers' commits from the branch (force-push it back to Autofixer's last commit); Autofixer then repairs and pushes as before. |
Problem
A scan cursor can sit between rows (
skip_advanceset) after a delete. If another cursor on the same connection then splits the leaf,try_save_position_for_external_balanceloses that position:Fix
The invariant: within one transaction, after the same connection writes the table, a forward scan must not return a row it already returned, and must not skip one.
Save
skip_advancecursors as a gap, not as an exact row:Every other
save_contextcaller usesGE eq_only=true, so this restore path only runs for these cursors. Index cursors with no record read the key cell first, then step back into the gap. Reading a column from a cursor in a gap returns NULL, which matches SQLite for a deleted row.This only changes the in-memory cursor state of the writing connection. Pages, WAL, locks, and MVCC are untouched.
Correctness argument
Tests
Prevafter a split, an emptied tree, and overflow index keys.All of them fail before the fix and pass after it. The whopper and limbo_sim seeds also pass, but they don't reach this code: the simulators never write a table while the same connection has an open scan on it.
Fixes #9324
Description of AI Usage
Autofixer wrote this change with Claude Code in an isolated sandbox. A separate verifier agent tested and reviewed it in its own container before this PR was opened. A maintainer must review it before merge.
To have Autofixer address review feedback, leave your comments and then mention @turso-github-handyman; it handles all unanswered comments from maintainers in one round.