From 47ef30b22e4505cd1bd7f4ad9452abc6dbef7e13 Mon Sep 17 00:00:00 2001 From: Jussi Saurio Date: Wed, 16 Jul 2025 10:05:02 +0300 Subject: [PATCH] btree: fix interior cell replacement in btrees with depth >=3 When a divider cell is deleted from an index interior page, the following algorithm is used: 1. Find predecessor: Move to largest key in left subtree (self.prev()) 2. Create replacement: Convert predecessor leaf cell to interior cell format, using original cell's left child pointer 3. Replace: Drop original cell from parent page, insert replacement at same position 4. Cleanup: Delete predecessor from leaf page The error in our logic was that we always expected to only traverse down one level of the btree: ```rust let parent_page = self.stack.parent_page().unwrap(); let leaf_page = self.stack.top(); ``` This meant that when the deletion happened on say, level 1, and the replacement cell was taken from level 3, we actually inserted the replacement cell into level 2 instead of level 1. In #2106, this manifested as the following chain of pages, going from parent to children: 3 -> 111 -> 119 Cell was deleted from page 3 (whose left pointer is 111), and a replacement cell was taken from 119, incorrectly inserted into 111, and its left child pointer also set as 111! The fix is quite trivial: store the page we are on before we start traversing down. Closes #2106 --- core/storage/btree.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index f54279a77..d40b360d6 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -4369,6 +4369,9 @@ impl BTreeCursor { // 3. Delete that key from the child page. // Step 1: Move cursor to the largest key in the left subtree. + // The largest key is always in a leaf, and so this traversal may involvegoing multiple pages downwards, + // so we store the page we are currently on. + let parent_page = self.stack.top(); return_if_io!(self.prev()); let (cell_payload, leaf_cell_idx) = { let leaf_page_ref = self.stack.top(); @@ -4409,7 +4412,6 @@ impl BTreeCursor { (cell_payload, leaf_cell_idx) }; - let parent_page = self.stack.parent_page().unwrap(); let leaf_page = self.stack.top(); parent_page.get().set_dirty(); @@ -4420,7 +4422,17 @@ impl BTreeCursor { // Step 2: Replace the cell in the parent (interior) page. { let parent_page_ref = parent_page.get(); - let parent_contents = parent_page_ref.get().contents.as_mut().unwrap(); + let parent_contents = parent_page_ref.get_contents(); + let parent_page_id = parent_page_ref.get().id; + let left_child_page = u32::from_be_bytes( + cell_payload[..4].try_into().expect("invalid cell payload"), + ); + turso_assert!( + left_child_page as usize != parent_page_id, + "corrupt: current page and left child page of cell {} are both {}", + left_child_page, + parent_page_id + ); // First, drop the old cell that is being replaced. drop_cell(parent_contents, cell_idx, self.usable_space() as u16)?; @@ -4436,7 +4448,7 @@ impl BTreeCursor { // Step 3: Delete the predecessor cell from the leaf page. { let leaf_page_ref = leaf_page.get(); - let leaf_contents = leaf_page_ref.get().contents.as_mut().unwrap(); + let leaf_contents = leaf_page_ref.get_contents(); drop_cell(leaf_contents, leaf_cell_idx, self.usable_space() as u16)?; } @@ -5539,18 +5551,6 @@ impl PageStack { fn clear(&self) { self.current_page.set(-1); } - pub fn parent_page(&self) -> Option { - if self.current_page.get() > 0 { - Some( - self.stack.borrow()[self.current() - 1] - .as_ref() - .unwrap() - .clone(), - ) - } else { - None - } - } } /// Used for redistributing cells during a balance operation.