diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index fd70ebbeb..ae5ad13a7 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -67,6 +67,7 @@ jobs: test-limbo: runs-on: blacksmith-4vcpu-ubuntu-2404 + timeout-minutes: 20 steps: - name: Install cargo-c env: diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 65f5588e6..65529793d 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -176,7 +176,9 @@ enum DeleteState { cell_idx: usize, original_child_pointer: Option, }, - CheckNeedsBalancing, + CheckNeedsBalancing { + rightmost_cell_was_dropped: bool, + }, WaitForBalancingToComplete { target_key: DeleteSavepoint, }, @@ -1117,8 +1119,8 @@ impl BTreeCursor { } loop { let mem_page_rc = self.stack.top(); - tracing::trace!( - "current_before_advance id={} cell={}", + tracing::debug!( + "next: current_before_advance id={} cell={}", mem_page_rc.get().get().id, self.stack.current_cell_index() ); @@ -1142,8 +1144,8 @@ impl BTreeCursor { // Important to advance only after loading the page in order to not advance > 1 times self.stack.advance(); let cell_idx = self.stack.current_cell_index() as usize; - tracing::trace!( - "current id={} cell={}", + tracing::debug!( + "next:current id={} cell={}", mem_page_rc.get().get().id, cell_idx ); @@ -1619,7 +1621,12 @@ impl BTreeCursor { assert!(self.mv_cursor.is_none()); let iter_dir = seek_op.iteration_direction(); - if matches!(self.seek_state, CursorSeekState::Start { .. }) { + if matches!( + self.seek_state, + CursorSeekState::Start { .. } + | CursorSeekState::MovingBetweenPages { .. } + | CursorSeekState::InteriorPageBinarySearch { .. } + ) { // No need for another move_to_root. Move_to already moves to root return_if_io!(self.move_to(SeekKey::TableRowId(rowid), seek_op)); let page = self.stack.top(); @@ -1733,7 +1740,12 @@ impl BTreeCursor { key: &ImmutableRecord, seek_op: SeekOp, ) -> Result> { - if matches!(self.seek_state, CursorSeekState::Start { .. }) { + if matches!( + self.seek_state, + CursorSeekState::Start { .. } + | CursorSeekState::MovingBetweenPages { .. } + | CursorSeekState::InteriorPageBinarySearch { .. } + ) { // No need for another move_to_root. Move_to already moves to root return_if_io!(self.move_to(SeekKey::IndexKey(key), seek_op)); let CursorSeekState::FoundLeaf { eq_seen } = &self.seek_state else { @@ -1785,7 +1797,10 @@ impl BTreeCursor { moving_up_to_parent, } = &self.seek_state else { - unreachable!("we must be in a leaf binary search state"); + unreachable!( + "we must be in a leaf binary search state, got: {:?}", + self.seek_state + ); }; if moving_up_to_parent.get() { @@ -2019,7 +2034,15 @@ impl BTreeCursor { // 5. We scan the leaf cells in the leaf page until we find the cell whose rowid is equal to the rowid we are looking for. // This cell contains the actual data we are looking for. // 6. If we find the cell, we return the record. Otherwise, we return an empty result. - if matches!(self.seek_state, CursorSeekState::Start { .. }) { + + // If we are at the beginning/end of seek state, start a new move from the root. + if matches!( + self.seek_state, + CursorSeekState::LeafPageBinarySearch { .. } + ) { + self.seek_state = CursorSeekState::Start; + } + if matches!(self.seek_state, CursorSeekState::Start) { self.move_to_root(); } @@ -4082,7 +4105,6 @@ impl BTreeCursor { // find_cell() iterates the leaf page from left to right to find the insertion point anyway, so we don't care // which cell we are in as long as we are on the right page. // FIXME: find_cell() should not use linear search because it's slow. - self.seek_state = CursorSeekState::Start; match key { BTreeKey::IndexKey(_) => { return_if_io!(self.move_to( @@ -4193,6 +4215,12 @@ impl BTreeCursor { )); } + tracing::debug!( + "DeleteState::FindCell: page_id: {}, cell_idx: {}", + page.get().id, + cell_idx + ); + let cell = contents.cell_get( cell_idx, payload_overflow_threshold_max( @@ -4231,6 +4259,8 @@ impl BTreeCursor { let page = page.get(); let contents = page.get_contents(); + let is_last_cell = cell_idx == contents.cell_count().saturating_sub(1); + let delete_info = self.state.mut_delete_info().unwrap(); if !contents.is_leaf() { delete_info.state = DeleteState::InteriorNodeReplacement { @@ -4242,7 +4272,9 @@ impl BTreeCursor { drop_cell(contents, cell_idx, self.usable_space() as u16)?; let delete_info = self.state.mut_delete_info().unwrap(); - delete_info.state = DeleteState::CheckNeedsBalancing; + delete_info.state = DeleteState::CheckNeedsBalancing { + rightmost_cell_was_dropped: is_last_cell, + }; } } @@ -4319,10 +4351,14 @@ impl BTreeCursor { drop_cell(parent_contents, cell_idx, self.usable_space() as u16)?; let delete_info = self.state.mut_delete_info().unwrap(); - delete_info.state = DeleteState::CheckNeedsBalancing; + delete_info.state = DeleteState::CheckNeedsBalancing { + rightmost_cell_was_dropped: false, // TODO: when is this true? + }; } - DeleteState::CheckNeedsBalancing => { + DeleteState::CheckNeedsBalancing { + rightmost_cell_was_dropped, + } => { let page = self.stack.top(); return_if_locked_maybe_load!(self.pager, page); @@ -4332,9 +4368,15 @@ impl BTreeCursor { let needs_balancing = self.stack.has_parent() && free_space as usize * 3 > self.usable_space() * 2; + if rightmost_cell_was_dropped { + // If we drop a cell in the middle, e.g. our current index is 2 and we drop 'c' from [a,b,c,d,e], then we don't need to retreat index, + // because index 2 is still going to be the right place [a,b,D,e] + // but: + // If we drop the rightmost cell, e.g. our current index is 4 and we drop 'e' from [a,b,c,d,e], then we need to retreat index, + // because index 4 is now pointing beyond the last cell [a,b,c,d] _ <-- index 4 + self.stack.retreat(); + } if needs_balancing { - // FIXME(pere): cell index must be updated before calling `rowid` or - // `record` let target_key = if page.is_index() { let record = match return_if_io!(self.record()) { Some(record) => record.clone(), diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 06ae3ea8d..f8be9e87d 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -870,9 +870,20 @@ fn emit_program_for_update( )?; // Open indexes for update. let mut index_cursors = Vec::with_capacity(plan.indexes_to_update.len()); - // TODO: do not reopen if there is table reference using it. - for index in &plan.indexes_to_update { + if let Some(index_cursor) = program.resolve_cursor_id_safe(&CursorKey::index( + plan.table_references + .joined_tables() + .first() + .unwrap() + .internal_id, + index.clone(), + )) { + // Don't reopen index if it was already opened as the iteration cursor for this update plan. + let record_reg = program.alloc_register(); + index_cursors.push((index_cursor, record_reg)); + continue; + } let index_cursor = program.alloc_cursor_id(CursorType::BTreeIndex(index.clone())); program.emit_insn(Insn::OpenWrite { cursor_id: index_cursor, diff --git a/core/translate/optimizer/mod.rs b/core/translate/optimizer/mod.rs index 38f1d3d73..910c93e2c 100644 --- a/core/translate/optimizer/mod.rs +++ b/core/translate/optimizer/mod.rs @@ -98,7 +98,7 @@ fn optimize_delete_plan(plan: &mut DeletePlan, schema: &Schema) -> Result<()> { Ok(()) } -fn optimize_update_plan(plan: &mut UpdatePlan, schema: &Schema) -> Result<()> { +fn optimize_update_plan(plan: &mut UpdatePlan, _schema: &Schema) -> Result<()> { rewrite_exprs_update(plan)?; if let ConstantConditionEliminationResult::ImpossibleCondition = eliminate_constant_conditions(&mut plan.where_clause)? @@ -106,13 +106,18 @@ fn optimize_update_plan(plan: &mut UpdatePlan, schema: &Schema) -> Result<()> { plan.contains_constant_false_condition = true; return Ok(()); } - let _ = optimize_table_access( - &mut plan.table_references, - &schema.indexes, - &mut plan.where_clause, - &mut plan.order_by, - &mut None, - )?; + // FIXME: don't use indexes for update right now because it's not safe to traverse an index + // while also updating the same table, things go wrong. + // e.g. in 'explain update t set x=x+5 where x > 10;' where x is an indexed column, + // sqlite first creates an ephemeral index to store the current values so the tree traversal + // doesn't get messed up while updating. + // let _ = optimize_table_access( + // &mut plan.table_references, + // &schema.indexes, + // &mut plan.where_clause, + // &mut plan.order_by, + // &mut None, + // )?; Ok(()) } diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index 5b1cce3bd..356025279 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -1345,11 +1345,16 @@ pub fn op_column( unreachable!("unexpected Insn {:?}", insn) }; if let Some((index_cursor_id, table_cursor_id)) = state.deferred_seeks[*cursor_id].take() { - let deferred_seek = { + let deferred_seek = 'd: { let rowid = { let mut index_cursor = state.get_cursor(index_cursor_id); let index_cursor = index_cursor.as_btree_mut(); - return_if_io!(index_cursor.rowid()) + match index_cursor.rowid()? { + CursorResult::IO => { + break 'd Some((index_cursor_id, table_cursor_id)); + } + CursorResult::Ok(rowid) => rowid, + } }; let mut table_cursor = state.get_cursor(table_cursor_id); let table_cursor = table_cursor.as_btree_mut(); @@ -1904,11 +1909,16 @@ pub fn op_row_id( unreachable!("unexpected Insn {:?}", insn) }; if let Some((index_cursor_id, table_cursor_id)) = state.deferred_seeks[*cursor_id].take() { - let deferred_seek = { + let deferred_seek = 'd: { let rowid = { let mut index_cursor = state.get_cursor(index_cursor_id); let index_cursor = index_cursor.as_btree_mut(); - let record = return_if_io!(index_cursor.record()); + let record = match index_cursor.record()? { + CursorResult::IO => { + break 'd Some((index_cursor_id, table_cursor_id)); + } + CursorResult::Ok(record) => record, + }; let record = record.as_ref().unwrap(); let rowid = record.get_values().last().unwrap(); match rowid { @@ -3903,11 +3913,6 @@ pub fn op_delete( { let mut cursor = state.get_cursor(*cursor_id); let cursor = cursor.as_btree_mut(); - tracing::debug!( - "op_delete(rowid ={:?}, record={:?})", - cursor.rowid()?, - cursor.record()?, - ); return_if_io!(cursor.delete()); } let prev_changes = program.n_change.get();