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
This commit is contained in:
Pere Diaz Bou
2025-04-10 15:05:20 +02:00
parent 4755acb571
commit 8e93471d00
2 changed files with 29 additions and 26 deletions

View File

@@ -1179,6 +1179,7 @@ impl BTreeCursor {
pub fn move_to(&mut self, key: SeekKey<'_>, cmp: SeekOp) -> Result<CursorResult<()>> {
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<CursorResult<()>> {
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 {

View File

@@ -3798,6 +3798,7 @@ pub fn op_idx_insert_async(
pager: &Rc<Pager>,
mv_store: Option<&Rc<MvStore>>,
) -> Result<InsnFunctionStepResult> {
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.