From 8e93471d0095211ca3c4d2a358b42bbbd16dbfd2 Mon Sep 17 00:00:00 2001 From: Pere Diaz Bou Date: Thu, 10 Apr 2025 15:05:20 +0200 Subject: [PATCH] fix cell index selection while balancing Cell index doesn't move in `move_to` unless we don't need to check next cell. On the other hand, with rightmost pointer, we advance cell index by 1 even though where we are moving to was to that page --- core/storage/btree.rs | 20 +++++++++++--------- core/vdbe/execute.rs | 35 ++++++++++++++++++----------------- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 8f4afb090..7b722d348 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -1179,6 +1179,7 @@ impl BTreeCursor { pub fn move_to(&mut self, key: SeekKey<'_>, cmp: SeekOp) -> Result> { assert!(self.mv_cursor.is_none()); tracing::trace!("move_to(key={:?} cmp={:?})", key, cmp); + tracing::trace!("backtrace: {}", std::backtrace::Backtrace::force_capture()); // For a table with N rows, we can find any row by row id in O(log(N)) time by starting at the root page and following the B-tree pointers. // B-trees consist of interior pages and leaf pages. Interior pages contain pointers to other pages, while leaf pages contain the actual row data. // @@ -1630,12 +1631,6 @@ impl BTreeCursor { let write_info = self.state.mut_write_info().unwrap(); write_info.state = WriteState::BalanceNonRoot; self.stack.pop(); - // with `move_to` we advance the current cell idx of TableInterior once we move to left subtree. - // On the other hand, with IndexInterior, we do not because we tranver in-order. In the latter case - // since we haven't consumed the cell we can avoid retreating the current cell index. - if matches!(current_page.get_contents().page_type(), PageType::TableLeaf) { - self.stack.retreat(); - } return_if_io!(self.balance_non_root()); } WriteState::BalanceNonRoot | WriteState::BalanceNonRootWaitLoadPages => { @@ -1660,10 +1655,14 @@ impl BTreeCursor { WriteState::BalanceStart => todo!(), WriteState::BalanceNonRoot => { let parent_page = self.stack.top(); - if parent_page.is_locked() { - return Ok(CursorResult::IO); - } return_if_locked_maybe_load!(self.pager, parent_page); + // If `move_to` moved to rightmost page, cell index will be out of bounds. Meaning cell_count+1. + // In any other case, `move_to` will stay in the correct index. + if self.stack.current_cell_index() as usize + == parent_page.get_contents().cell_count() + 1 + { + self.stack.retreat(); + } parent_page.set_dirty(); self.pager.add_dirty(parent_page.get().id); let parent_contents = parent_page.get().contents.as_ref().unwrap(); @@ -2871,6 +2870,7 @@ impl BTreeCursor { &mut child_contents.overflow_cells, &mut root_contents.overflow_cells, ); + root_contents.overflow_cells.clear(); // 2. Modify root let new_root_page_type = match root_contents.page_type() { @@ -3133,6 +3133,7 @@ impl BTreeCursor { key: &BTreeKey, moved_before: bool, /* Indicate whether it's necessary to traverse to find the leaf page */ ) -> Result> { + tracing::trace!("insert"); match &self.mv_cursor { Some(mv_cursor) => match key.maybe_rowid() { Some(rowid) => { @@ -3144,6 +3145,7 @@ impl BTreeCursor { None => todo!("Support mvcc inserts with index btrees"), }, None => { + tracing::trace!("moved {}", moved_before); if !moved_before { self.iteration_state = IterationState::Iterating(IterationDirection::Forwards); match key { diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 2b71ee716..6827ba83b 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -3798,6 +3798,7 @@ pub fn op_idx_insert_async( pager: &Rc, mv_store: Option<&Rc>, ) -> Result { + dbg!("op_idx_insert_async"); if let Insn::IdxInsertAsync { cursor_id, record_reg, @@ -3816,29 +3817,29 @@ pub fn op_idx_insert_async( Register::Record(ref r) => r, _ => return Err(LimboError::InternalError("expected record".into())), }; - let moved_before = if index_meta.unique { - // check for uniqueness violation - match cursor.key_exists_in_index(record)? { - CursorResult::Ok(true) => { - return Err(LimboError::Constraint( - "UNIQUE constraint failed: duplicate key".into(), - )) - } - CursorResult::IO => return Ok(InsnFunctionStepResult::IO), - CursorResult::Ok(false) => {} - }; - false - } else { - flags.has(IdxInsertFlags::USE_SEEK) - }; - // To make this reentrant in case of `moved_before` = false, we need to check if the previous cursor.insert started // a write/balancing operation. If it did, it means we already moved to the place we wanted. let moved_before = if cursor.is_write_in_progress() { true } else { - moved_before + if index_meta.unique { + // check for uniqueness violation + match cursor.key_exists_in_index(record)? { + CursorResult::Ok(true) => { + return Err(LimboError::Constraint( + "UNIQUE constraint failed: duplicate key".into(), + )) + } + CursorResult::IO => return Ok(InsnFunctionStepResult::IO), + CursorResult::Ok(false) => {} + }; + false + } else { + flags.has(IdxInsertFlags::USE_SEEK) + } }; + + dbg!(moved_before); // Start insertion of row. This might trigger a balance procedure which will take care of moving to different pages, // therefore, we don't want to seek again if that happens, meaning we don't want to return on io without moving to `Await` opcode // because it could trigger a movement to child page after a balance root which will leave the current page as the root page.