diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 09abcc2dd..6db1efce6 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -463,9 +463,6 @@ pub enum CursorSeekState { /// 1. We have not seen an EQ during the traversal /// 2. We are looking for an exact match ([SeekOp::GE] or [SeekOp::LE] with eq_only: true) eq_seen: Cell, - /// Indicates when we have not not found a value in leaf and now will look in the next/prev record. - /// This value is only used for indexbtree - moving_up_to_parent: Cell, }, } @@ -1291,15 +1288,7 @@ impl BTreeCursor { self.tablebtree_seek(rowid, op) } SeekKey::IndexKey(index_key) => { - match self.indexbtree_seek(index_key, op) { - Ok(CursorResult::Ok(found)) => Ok(CursorResult::Ok(if found { - SeekResult::Found - } else { - SeekResult::NotFound - })), - Ok(CursorResult::IO) => Ok(CursorResult::IO), - Err(err) => Err(err), - } + self.indexbtree_seek(index_key, op) } }); self.valid_state = CursorValidState::Valid; @@ -1725,7 +1714,6 @@ impl BTreeCursor { min_cell_idx, max_cell_idx, nearest_matching_cell, - moving_up_to_parent: Cell::new(false), eq_seen: Cell::new(false), // not relevant for table btrees }; } @@ -1828,7 +1816,7 @@ impl BTreeCursor { &mut self, key: &ImmutableRecord, seek_op: SeekOp, - ) -> Result> { + ) -> Result> { let key_values = key.get_values(); let index_info_default = IndexKeyInfo::default(); let index_info = *self.index_key_info.as_ref().unwrap_or(&index_info_default); @@ -1861,15 +1849,7 @@ impl BTreeCursor { let contents = page.get().contents.as_ref().unwrap(); let cell_count = contents.cell_count(); if cell_count == 0 { - self.stack.set_cell_index(0); - match seek_op.iteration_direction() { - IterationDirection::Forwards => { - return self.next(); - } - IterationDirection::Backwards => { - return self.prev(); - } - } + return Ok(CursorResult::Ok(SeekResult::NotFound)); } let min = Cell::new(0); @@ -1883,7 +1863,6 @@ impl BTreeCursor { min_cell_idx: min, max_cell_idx: max, nearest_matching_cell, - moving_up_to_parent: Cell::new(false), eq_seen: Cell::new(eq_seen), }; } @@ -1893,7 +1872,6 @@ impl BTreeCursor { max_cell_idx, nearest_matching_cell, eq_seen, - moving_up_to_parent, } = &self.seek_state else { unreachable!( @@ -1902,47 +1880,6 @@ impl BTreeCursor { ); }; - if moving_up_to_parent.get() { - let page = self.stack.top(); - return_if_locked_maybe_load!(self.pager, page); - let page = page.get(); - let contents = page.get().contents.as_ref().unwrap(); - let cur_cell_idx = self.stack.current_cell_index() as usize; - let cell = contents.cell_get(cur_cell_idx, self.usable_space())?; - let BTreeCell::IndexInteriorCell(IndexInteriorCell { - payload, - first_overflow_page, - payload_size, - .. - }) = &cell - else { - unreachable!("unexpected cell type: {:?}", cell); - }; - - if let Some(next_page) = first_overflow_page { - return_if_io!(self.process_overflow_read(payload, *next_page, *payload_size)) - } else { - self.get_immutable_record_or_create() - .as_mut() - .unwrap() - .invalidate(); - self.get_immutable_record_or_create() - .as_mut() - .unwrap() - .start_serialization(payload); - - self.record_cursor.borrow_mut().invalidate(); - }; - let (_, found) = self.compare_with_current_record( - key_values.as_slice(), - seek_op, - &record_comparer, - &index_info, - ); - moving_up_to_parent.set(false); - return Ok(CursorResult::Ok(found)); - } - let page = self.stack.top(); return_if_locked_maybe_load!(self.pager, page); let page = page.get(); @@ -1957,56 +1894,20 @@ impl BTreeCursor { if min > max { if let Some(nearest_matching_cell) = nearest_matching_cell.get() { self.stack.set_cell_index(nearest_matching_cell as i32); - return Ok(CursorResult::Ok(true)); + return Ok(CursorResult::Ok(SeekResult::Found)); } else { - // We have now iterated over all cells in the leaf page and found no match. - // Unlike tables, indexes store payloads in interior cells as well. self.move_to() always moves to a leaf page, so there are cases where we need to - // move back up to the parent interior cell and get the next record from there to perform a correct seek. - // an example of how this can occur: - // - // we do an index seek for key K with cmp = SeekOp::GT, meaning we want to seek to the first key that is greater than K. - // in self.move_to(), we encounter an interior cell with key K' = K+2, and move the left child page, which is a leaf page. - // the reason we move to the left child page is that we know that in an index, all keys in the left child page are less than K' i.e. less than K+2, - // meaning that the left subtree may contain a key greater than K, e.g. K+1. however, it is possible that it doesn't, in which case the correct - // next key is K+2, which is in the parent interior cell. - // - // In the seek() method, once we have landed in the leaf page and find that there is no cell with a key greater than K, - // if we were to return Ok(CursorResult::Ok((None, None))), self.record would be None, which is incorrect, because we already know - // that there is a record with a key greater than K (K' = K+2) in the parent interior cell. Hence, we need to move back up the tree - // and get the next matching record from there. - // - // However we only do this if either of the following is true: - // - We have seen an EQ match up in the tree in an interior node - // - Or, we are not looking for an exact match. + // Similar logic as in tablebtree_seek(), but for indexes. + // The difference is that since index keys are not necessarily unique, we need to TryAdvance + // even when eq_only=true and we have seen an EQ match up in the tree in an interior node. if seek_op.eq_only() && !eq_seen.get() { - return Ok(CursorResult::Ok(false)); - } - match iter_dir { - IterationDirection::Forwards => { - if !moving_up_to_parent.get() { - moving_up_to_parent.set(true); - self.stack.set_cell_index(cell_count as i32); - } - let next_res = return_if_io!(self.next()); - if !next_res { - return Ok(CursorResult::Ok(false)); - } - // FIXME: optimize this in case record can be read directly - return Ok(CursorResult::IO); - } - IterationDirection::Backwards => { - if !moving_up_to_parent.get() { - moving_up_to_parent.set(true); - self.stack.set_cell_index(-1); - } - let prev_res = return_if_io!(self.prev()); - if !prev_res { - return Ok(CursorResult::Ok(false)); - } - // FIXME: optimize this in case record can be read directly - return Ok(CursorResult::IO); - } + return Ok(CursorResult::Ok(SeekResult::NotFound)); } + // set cursor to the position where which would hold the op-boundary if it were present + self.stack.set_cell_index(match &seek_op { + SeekOp::GT | SeekOp::GE { .. } => cell_count as i32, + SeekOp::LT | SeekOp::LE { .. } => 0, + }); + return Ok(CursorResult::Ok(SeekResult::TryAdvance)); }; } @@ -7164,10 +7065,11 @@ mod tests { pager.deref(), ) .unwrap(); - assert!( - matches!(exists, SeekResult::Found), - "key {key:?} is not found" - ); + let mut found = matches!(exists, SeekResult::Found); + if matches!(exists, SeekResult::TryAdvance) { + found = run_until_done(|| cursor.next(), pager.deref()).unwrap(); + } + assert!(found, "key {key:?} is not found"); } // Check that key count is right cursor.move_to_root().unwrap(); diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index c66a318a8..9113c9650 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -2392,11 +2392,22 @@ pub fn op_deferred_seek( Ok(InsnFunctionStepResult::Step) } +/// Separate enum for seek key to avoid lifetime issues +/// with using [SeekKey] - OpSeekState always owns the key, +/// unless it's [OpSeekKey::IndexKeyFromRegister] in which case the record +/// is owned by the program state's registers and we store the register number. +#[derive(Debug)] +pub enum OpSeekKey { + TableRowId(i64), + IndexKeyOwned(ImmutableRecord), + IndexKeyFromRegister(usize), +} + pub enum OpSeekState { /// Initial state Start, /// Position cursor with seek operation with (rowid, op) search parameters - Seek { rowid: i64, op: SeekOp }, + Seek { key: OpSeekKey, op: SeekOp }, /// Advance cursor (with [BTreeCursor::next]/[BTreeCursor::prev] methods) which was /// positioned after [OpSeekState::Seek] state if [BTreeCursor::seek] returned [SeekResult::TryAdvance] Advance { op: SeekOp }, @@ -2411,56 +2422,52 @@ pub fn op_seek( pager: &Rc, mv_store: Option<&Rc>, ) -> Result { - let result = op_seek_internal(program, state, insn, pager, mv_store); - if let Err(_) | Ok(InsnFunctionStepResult::Step) = &result { - state.op_seek_state = OpSeekState::Start; - } - result -} - -pub fn op_seek_internal( - program: &Program, - state: &mut ProgramState, - insn: &Insn, - pager: &Rc, - mv_store: Option<&Rc>, -) -> Result { - let (Insn::SeekGE { - cursor_id, - start_reg, - num_regs, - target_pc, - is_index, - .. - } - | Insn::SeekGT { - cursor_id, - start_reg, - num_regs, - target_pc, - is_index, - } - | Insn::SeekLE { - cursor_id, - start_reg, - num_regs, - target_pc, - is_index, - .. - } - | Insn::SeekLT { - cursor_id, - start_reg, - num_regs, - target_pc, - is_index, - }) = insn - else { - unreachable!("unexpected Insn {:?}", insn) + let (cursor_id, is_index, record_source, target_pc) = match insn { + Insn::SeekGE { + cursor_id, + is_index, + start_reg, + num_regs, + target_pc, + .. + } + | Insn::SeekLE { + cursor_id, + is_index, + start_reg, + num_regs, + target_pc, + .. + } + | Insn::SeekGT { + cursor_id, + is_index, + start_reg, + num_regs, + target_pc, + .. + } + | Insn::SeekLT { + cursor_id, + is_index, + start_reg, + num_regs, + target_pc, + .. + } => ( + cursor_id, + *is_index, + RecordSource::Unpacked { + start_reg: *start_reg, + num_regs: *num_regs, + }, + target_pc, + ), + _ => unreachable!("unexpected Insn {:?}", insn), }; assert!( target_pc.is_offset(), - "target_pc should be an offset, is: {target_pc:?}" + "op_seek: target_pc should be an offset, is: {target_pc:?}" ); let eq_only = match insn { Insn::SeekGE { eq_only, .. } | Insn::SeekLE { eq_only, .. } => *eq_only, @@ -2473,169 +2480,280 @@ pub fn op_seek_internal( Insn::SeekLT { .. } => SeekOp::LT, _ => unreachable!("unexpected Insn {:?}", insn), }; - if *is_index { - let seek_result = { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); - let record_from_regs = make_record(&state.registers, start_reg, num_regs); - return_if_io!(cursor.seek(SeekKey::IndexKey(&record_from_regs), op)) - }; - if !matches!(seek_result, SeekResult::Found) { - state.pc = target_pc.as_offset_int(); - } else { + match seek_internal( + program, + state, + pager, + mv_store, + record_source, + *cursor_id, + is_index, + op, + ) { + Ok(SeekInternalResult::Found) => { state.pc += 1; + Ok(InsnFunctionStepResult::Step) } - return Ok(InsnFunctionStepResult::Step); + Ok(SeekInternalResult::NotFound) => { + state.pc = target_pc.as_offset_int(); + Ok(InsnFunctionStepResult::Step) + } + Ok(SeekInternalResult::IO) => Ok(InsnFunctionStepResult::IO), + Err(e) => Err(e), } - loop { - match &state.op_seek_state { - OpSeekState::Start => { - let original_value = state.registers[*start_reg].get_owned_value().clone(); - let mut temp_value = original_value.clone(); +} - let conversion_successful = if matches!(temp_value, Value::Text(_)) { - let mut temp_reg = Register::Value(temp_value); - let converted = apply_numeric_affinity(&mut temp_reg, false); - temp_value = temp_reg.get_owned_value().clone(); - converted - } else { - true // Non-text values don't need conversion - }; - let int_key = extract_int_value(&temp_value); - let lost_precision = - !conversion_successful || !matches!(temp_value, Value::Integer(_)); - let actual_op = if lost_precision { - match &temp_value { - Value::Float(f) => { - let int_key_as_float = int_key as f64; - let c = if int_key_as_float > *f { - 1 - } else if int_key_as_float < *f { - -1 - } else { - 0 - }; +#[derive(Debug, PartialEq)] +pub enum SeekInternalResult { + Found, + NotFound, + IO, +} +pub enum RecordSource { + Unpacked { start_reg: usize, num_regs: usize }, + Packed { record_reg: usize }, +} - match c.cmp(&0) { - std::cmp::Ordering::Less => match op { - SeekOp::LT => SeekOp::LE { eq_only: false }, // (x < 5.1) -> (x <= 5) - SeekOp::GE { .. } => SeekOp::GT, // (x >= 5.1) -> (x > 5) - other => other, - }, - std::cmp::Ordering::Greater => match op { - SeekOp::GT => SeekOp::GE { eq_only: false }, // (x > 4.9) -> (x >= 5) - SeekOp::LE { .. } => SeekOp::LT, // (x <= 4.9) -> (x < 5) - other => other, - }, - std::cmp::Ordering::Equal => op, +/// Internal function used by many VDBE instructions that need to perform a seek operation. +/// +/// Explanation for some of the arguments: +/// - `record_source`: whether the seek key record is already a record (packed) or it will be constructed from registers (unpacked) +/// - `cursor_id`: the cursor id +/// - `is_index`: true if the cursor is an index, false if it is a table +/// - `op`: the [SeekOp] to perform +#[allow(clippy::too_many_arguments)] +pub fn seek_internal( + program: &Program, + state: &mut ProgramState, + pager: &Rc, + mv_store: Option<&Rc>, + record_source: RecordSource, + cursor_id: usize, + is_index: bool, + op: SeekOp, +) -> Result { + /// wrapper so we can use the ? operator and handle errors correctly in this outer function + fn inner( + program: &Program, + state: &mut ProgramState, + pager: &Rc, + mv_store: Option<&Rc>, + record_source: RecordSource, + cursor_id: usize, + is_index: bool, + op: SeekOp, + ) -> Result { + loop { + match &state.seek_state { + OpSeekState::Start => { + if is_index { + // FIXME: text-to-numeric conversion should also happen here when applicable (when index column is numeric) + // See below for the table-btree implementation of this + match record_source { + RecordSource::Unpacked { + start_reg, + num_regs, + } => { + let record_from_regs = + make_record(&state.registers, &start_reg, &num_regs); + state.seek_state = OpSeekState::Seek { + key: OpSeekKey::IndexKeyOwned(record_from_regs), + op, + }; } - } - Value::Text(_) | Value::Blob(_) => { - match op { - SeekOp::GT | SeekOp::GE { .. } => { - // No integers are > or >= non-numeric text, jump to target (empty result) - state.pc = target_pc.as_offset_int(); - return Ok(InsnFunctionStepResult::Step); - } - SeekOp::LT | SeekOp::LE { .. } => { - // All integers are < or <= non-numeric text - // Move to last position and then use the normal seek logic - state.op_seek_state = OpSeekState::MoveLast; - continue; - } + RecordSource::Packed { record_reg } => { + state.seek_state = OpSeekState::Seek { + key: OpSeekKey::IndexKeyFromRegister(record_reg), + op, + }; } - } - _ => op, - } - } else { - op - }; - let rowid = if matches!(original_value, Value::Null) { - match actual_op { - SeekOp::GE { .. } | SeekOp::GT => { - state.pc = target_pc.as_offset_int(); - return Ok(InsnFunctionStepResult::Step); - } - SeekOp::LE { .. } | SeekOp::LT => { - // No integers are < NULL, so jump to target - state.pc = target_pc.as_offset_int(); - return Ok(InsnFunctionStepResult::Step); - } - } - } else { - int_key - }; - state.op_seek_state = OpSeekState::Seek { - rowid, - op: actual_op, - }; - continue; - } - OpSeekState::Seek { rowid, op } => { - let seek_result = { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); - return_if_io!(cursor.seek(SeekKey::TableRowId(*rowid), *op)) - }; - let found = match seek_result { - SeekResult::Found => true, - SeekResult::NotFound => false, - SeekResult::TryAdvance => { - state.op_seek_state = OpSeekState::Advance { op: *op }; + }; continue; } - }; - if !found { - state.pc = target_pc.as_offset_int() - } else { - state.pc += 1 + let RecordSource::Unpacked { + start_reg, + num_regs, + } = record_source + else { + unreachable!("op_seek: record_source should be Unpacked for table-btree"); + }; + assert_eq!(num_regs, 1, "op_seek: num_regs should be 1 for table-btree"); + let original_value = state.registers[start_reg].get_owned_value().clone(); + let mut temp_value = original_value.clone(); + + let conversion_successful = if matches!(temp_value, Value::Text(_)) { + let mut temp_reg = Register::Value(temp_value); + let converted = apply_numeric_affinity(&mut temp_reg, false); + temp_value = temp_reg.get_owned_value().clone(); + converted + } else { + true // Non-text values don't need conversion + }; + let int_key = extract_int_value(&temp_value); + let lost_precision = + !conversion_successful || !matches!(temp_value, Value::Integer(_)); + let actual_op = if lost_precision { + match &temp_value { + Value::Float(f) => { + let int_key_as_float = int_key as f64; + let c = if int_key_as_float > *f { + 1 + } else if int_key_as_float < *f { + -1 + } else { + 0 + }; + + match c.cmp(&0) { + std::cmp::Ordering::Less => match op { + SeekOp::LT => SeekOp::LE { eq_only: false }, // (x < 5.1) -> (x <= 5) + SeekOp::GE { .. } => SeekOp::GT, // (x >= 5.1) -> (x > 5) + other => other, + }, + std::cmp::Ordering::Greater => match op { + SeekOp::GT => SeekOp::GE { eq_only: false }, // (x > 4.9) -> (x >= 5) + SeekOp::LE { .. } => SeekOp::LT, // (x <= 4.9) -> (x < 5) + other => other, + }, + std::cmp::Ordering::Equal => op, + } + } + Value::Text(_) | Value::Blob(_) => { + match op { + SeekOp::GT | SeekOp::GE { .. } => { + // No integers are > or >= non-numeric text + return Ok(SeekInternalResult::NotFound); + } + SeekOp::LT | SeekOp::LE { .. } => { + // All integers are < or <= non-numeric text + // Move to last position and then use the normal seek logic + state.seek_state = OpSeekState::MoveLast; + continue; + } + } + } + _ => op, + } + } else { + op + }; + let rowid = if matches!(original_value, Value::Null) { + match actual_op { + SeekOp::GE { .. } | SeekOp::GT => { + return Ok(SeekInternalResult::NotFound); + } + SeekOp::LE { .. } | SeekOp::LT => { + // No integers are < NULL + return Ok(SeekInternalResult::NotFound); + } + } + } else { + int_key + }; + state.seek_state = OpSeekState::Seek { + key: OpSeekKey::TableRowId(rowid), + op: actual_op, + }; + continue; } - return Ok(InsnFunctionStepResult::Step); - } - OpSeekState::Advance { op } => { - let found = { - let mut cursor = state.get_cursor(*cursor_id); + OpSeekState::Seek { key, op } => { + let seek_result = { + let mut cursor = state.get_cursor(cursor_id); + let cursor = cursor.as_btree_mut(); + let seek_key = match key { + OpSeekKey::TableRowId(rowid) => SeekKey::TableRowId(*rowid), + OpSeekKey::IndexKeyOwned(record) => SeekKey::IndexKey(record), + OpSeekKey::IndexKeyFromRegister(record_reg) => match &state.registers[*record_reg] { + Register::Record(ref record) => SeekKey::IndexKey(record), + _ => unreachable!("op_seek: record_reg should be a Record register when OpSeekKey::IndexKeyFromRegister is used"), + } + }; + match cursor.seek(seek_key, *op)? { + CursorResult::Ok(seek_result) => seek_result, + CursorResult::IO => return Ok(SeekInternalResult::IO), + } + }; + let found = match seek_result { + SeekResult::Found => true, + SeekResult::NotFound => false, + SeekResult::TryAdvance => { + state.seek_state = OpSeekState::Advance { op: *op }; + continue; + } + }; + return Ok(if found { + SeekInternalResult::Found + } else { + SeekInternalResult::NotFound + }); + } + OpSeekState::Advance { op } => { + let found = { + let mut cursor = state.get_cursor(cursor_id); + let cursor = cursor.as_btree_mut(); + // Seek operation has anchor number which equals to the closed boundary of the range + // (e.g. for >= x - anchor is x, for > x - anchor is x + 1) + // + // Before Advance state, cursor was positioned to the leaf page which should hold the anchor. + // Sometimes this leaf page can have no matching rows, and in this case + // we need to move cursor in the direction of Seek to find record which matches the seek filter + // + // Consider following scenario: Seek [> 666] + // interior page dividers: I1: [ .. 667 .. ] + // / \ + // leaf pages: P1[661,665] P2[anything here is GT 666] + // After the initial Seek, cursor will be positioned after the end of leaf page P1 [661, 665] + // because this is potential position for insertion of value 666. + // But as P1 has no row matching Seek criteria - we need to move it to the right + // (and as we at the page boundary, we will move cursor to the next neighbor leaf, which guaranteed to have + // row keys greater than divider, which is greater or equal than anchor) + + // this same logic applies for indexes, but the next/prev record is expected to be found in the parent page's + // divider cell. + let result = match op { + SeekOp::GT { .. } | SeekOp::GE { .. } => cursor.next()?, + SeekOp::LT { .. } | SeekOp::LE { .. } => cursor.prev()?, + }; + match result { + CursorResult::Ok(found) => found, + CursorResult::IO => return Ok(SeekInternalResult::IO), + } + }; + return Ok(if found { + SeekInternalResult::Found + } else { + SeekInternalResult::NotFound + }); + } + OpSeekState::MoveLast => { + let mut cursor = state.get_cursor(cursor_id); let cursor = cursor.as_btree_mut(); - // Seek operation has anchor number which equals to the closed boundary of the range - // (e.g. for >= x - anchor is x, for > x - anchor is x + 1) - // - // Before Advance state, cursor was positioned to the leaf page which should hold the anchor. - // Sometimes this leaf page can have no matching rows, and in this case - // we need to move cursor in the direction of Seek to find record which matches the seek filter - // - // Consider following scenario: Seek [> 666] - // interior page dividers: I1: [ .. 667 .. ] - // / \ - // leaf pages: P1[661,665] P2[anything here is GT 666] - // After the initial Seek, cursor will be positioned after the end of leaf page P1 [661, 665] - // because this is potential position for insertion of value 666. - // But as P1 has no row matching Seek criteria - we need to move it to the right - // (and as we at the page boundary, we will move cursor to the next neighbor leaf, which guaranteed to have - // row keys greater than divider, which is greater or equal than anchor) - match op { - SeekOp::GT | SeekOp::GE { eq_only: false } => return_if_io!(cursor.next()), - SeekOp::LT | SeekOp::LE { eq_only: false } => return_if_io!(cursor.prev()), - _ => unreachable!("eq_only: true state must be unreachable"), + match cursor.last()? { + CursorResult::Ok(()) => {} + CursorResult::IO => return Ok(SeekInternalResult::IO), } - }; - if !found { - state.pc = target_pc.as_offset_int() - } else { - state.pc += 1 + // the MoveLast variant is only used for SeekOp::LT and SeekOp::LE when the seek condition is always true, + // so we have always found what we were looking for. + return Ok(SeekInternalResult::Found); } - return Ok(InsnFunctionStepResult::Step); - } - OpSeekState::MoveLast => { - { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); - return_if_io!(cursor.last()); - } - state.pc += 1; - return Ok(InsnFunctionStepResult::Step); } } } + + let result = inner( + program, + state, + pager, + mv_store, + record_source, + cursor_id, + is_index, + op, + ); + if !matches!(result, Ok(SeekInternalResult::IO)) { + state.seek_state = OpSeekState::Start; + } + result } /// Returns the tie-breaker ordering for SQLite index comparison opcodes. @@ -4837,7 +4955,7 @@ pub fn op_delete( #[derive(Debug)] pub enum OpIdxDeleteState { - Seeking(ImmutableRecord), // First seek row to delete + Seeking, Verifying, Deleting, } @@ -4857,6 +4975,7 @@ pub fn op_idx_delete( else { unreachable!("unexpected Insn {:?}", insn) }; + loop { tracing::debug!( "op_idx_delete(cursor_id={}, start_reg={}, num_regs={}, rootpage={}, state={:?})", @@ -4867,26 +4986,31 @@ pub fn op_idx_delete( state.op_idx_delete_state ); match &state.op_idx_delete_state { - Some(OpIdxDeleteState::Seeking(record)) => { - let found = { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); - let seek_result = return_if_io!( - cursor.seek(SeekKey::IndexKey(record), SeekOp::GE { eq_only: true }) - ); - tracing::debug!( - "op_idx_delete: found={:?}, rootpage={}, key={:?}", - seek_result, - cursor.root_page(), - record - ); - matches!(seek_result, SeekResult::Found) + Some(OpIdxDeleteState::Seeking) => { + let found = match seek_internal( + program, + state, + pager, + mv_store, + RecordSource::Unpacked { + start_reg: *start_reg, + num_regs: *num_regs, + }, + *cursor_id, + true, + SeekOp::GE { eq_only: true }, + ) { + Ok(SeekInternalResult::Found) => true, + Ok(SeekInternalResult::NotFound) => false, + Ok(SeekInternalResult::IO) => return Ok(InsnFunctionStepResult::IO), + Err(e) => return Err(e), }; if !found { // If P5 is not zero, then raise an SQLITE_CORRUPT_INDEX error if no matching index entry is found // Also, do not raise this (self-correcting and non-critical) error if in writable_schema mode. if *raise_error_if_no_matching_entry { + let record = make_record(&state.registers, start_reg, num_regs); return Err(LimboError::Corrupt(format!( "IdxDelete: no matching index entry found for record {record:?}" ))); @@ -4925,8 +5049,7 @@ pub fn op_idx_delete( return Ok(InsnFunctionStepResult::Step); } None => { - let record = make_record(&state.registers, start_reg, num_regs); - state.op_idx_delete_state = Some(OpIdxDeleteState::Seeking(record)); + state.op_idx_delete_state = Some(OpIdxDeleteState::Seeking); } } } @@ -4981,17 +5104,27 @@ pub fn op_idx_insert( }; return Ok(InsnFunctionStepResult::Step); } - { - let mut cursor = state.get_cursor(cursor_id); - let cursor = cursor.as_btree_mut(); - return_if_io!(cursor.seek( - SeekKey::IndexKey(record_to_insert), - SeekOp::GE { eq_only: true } - )); + match seek_internal( + program, + state, + pager, + mv_store, + RecordSource::Packed { record_reg }, + cursor_id, + true, + SeekOp::GE { eq_only: true }, + )? { + SeekInternalResult::Found => { + state.op_idx_insert_state = OpIdxInsertState::UniqueConstraintCheck; + Ok(InsnFunctionStepResult::Step) + } + SeekInternalResult::NotFound => { + state.op_idx_insert_state = OpIdxInsertState::Insert { moved_before: true }; + Ok(InsnFunctionStepResult::Step) + } + SeekInternalResult::IO => Ok(InsnFunctionStepResult::IO), } - state.op_idx_insert_state = OpIdxInsertState::UniqueConstraintCheck; - Ok(InsnFunctionStepResult::Step) } OpIdxInsertState::UniqueConstraintCheck => { let ignore_conflict = 'i: { @@ -5239,6 +5372,10 @@ pub fn op_soft_null( Ok(InsnFunctionStepResult::Step) } +/// If a matching record is not found in the btree ("no conflict"), jump to the target PC. +/// Otherwise, continue execution. +/// FIXME: create a state machine for op_no_conflict so that it doesn't do the nulls checking +/// multiple times in the case of yielding on IO. pub fn op_no_conflict( program: &Program, state: &mut ProgramState, @@ -5258,39 +5395,67 @@ pub fn op_no_conflict( let mut cursor_ref = state.get_cursor(*cursor_id); let cursor = cursor_ref.as_btree_mut(); - let record = if *num_regs == 0 { - match &state.registers[*record_reg] { - Register::Record(r) => r, - _ => { - return Err(LimboError::InternalError( - "NoConflict: exepected a record in the register".into(), - )); - } + let record_source = if *num_regs == 0 { + RecordSource::Packed { + record_reg: *record_reg, } } else { - &make_record(&state.registers, record_reg, num_regs) + RecordSource::Unpacked { + start_reg: *record_reg, + num_regs: *num_regs, + } }; + // If there is at least one NULL in the index record, there cannot be a conflict so we can immediately jump. - let contains_nulls = record - .get_values() - .iter() - .any(|val| matches!(val, RefValue::Null)); + let contains_nulls = match &record_source { + RecordSource::Packed { record_reg } => { + let Register::Record(record) = &state.registers[*record_reg] else { + return Err(LimboError::InternalError( + "NoConflict: expected a record in the register".into(), + )); + }; + record + .get_values() + .iter() + .any(|val| matches!(val, RefValue::Null)) + } + RecordSource::Unpacked { + start_reg, + num_regs, + } => (0..*num_regs).any(|i| { + matches!( + &state.registers[start_reg + i], + Register::Value(Value::Null) + ) + }), + }; if contains_nulls { drop(cursor_ref); state.pc = target_pc.as_offset_int(); return Ok(InsnFunctionStepResult::Step); } - - let seek_result = - return_if_io!(cursor.seek(SeekKey::IndexKey(record), SeekOp::GE { eq_only: true })); drop(cursor_ref); - if !matches!(seek_result, SeekResult::Found) { - state.pc = target_pc.as_offset_int(); - } else { - state.pc += 1; + match seek_internal( + program, + state, + pager, + mv_store, + record_source, + *cursor_id, + true, + SeekOp::GE { eq_only: true }, + )? { + SeekInternalResult::Found => { + state.pc += 1; + Ok(InsnFunctionStepResult::Step) + } + SeekInternalResult::NotFound => { + state.pc = target_pc.as_offset_int(); + Ok(InsnFunctionStepResult::Step) + } + SeekInternalResult::IO => Ok(InsnFunctionStepResult::IO), } - Ok(InsnFunctionStepResult::Step) } pub fn op_not_exists( @@ -6099,25 +6264,30 @@ pub fn op_found( let not = matches!(insn, Insn::NotFound { .. }); - let seek_result = { - let mut cursor = state.get_cursor(*cursor_id); - let cursor = cursor.as_btree_mut(); - - if *num_regs == 0 { - let record = match &state.registers[*record_reg] { - Register::Record(r) => r, - _ => { - return Err(LimboError::InternalError( - "NotFound: exepected a record in the register".into(), - )); - } - }; - - return_if_io!(cursor.seek(SeekKey::IndexKey(record), SeekOp::GE { eq_only: true })) - } else { - let record = make_record(&state.registers, record_reg, num_regs); - return_if_io!(cursor.seek(SeekKey::IndexKey(&record), SeekOp::GE { eq_only: true })) + let record_source = if *num_regs == 0 { + RecordSource::Packed { + record_reg: *record_reg, } + } else { + RecordSource::Unpacked { + start_reg: *record_reg, + num_regs: *num_regs, + } + }; + let seek_result = match seek_internal( + program, + state, + pager, + mv_store, + record_source, + *cursor_id, + true, + SeekOp::GE { eq_only: true }, + ) { + Ok(SeekInternalResult::Found) => SeekResult::Found, + Ok(SeekInternalResult::NotFound) => SeekResult::NotFound, + Ok(SeekInternalResult::IO) => return Ok(InsnFunctionStepResult::IO), + Err(e) => return Err(e), }; let found = matches!(seek_result, SeekResult::Found); diff --git a/core/vdbe/mod.rs b/core/vdbe/mod.rs index d79e88845..415da0b39 100644 --- a/core/vdbe/mod.rs +++ b/core/vdbe/mod.rs @@ -258,7 +258,7 @@ pub struct ProgramState { op_new_rowid_state: OpNewRowidState, op_idx_insert_state: OpIdxInsertState, op_insert_state: OpInsertState, - op_seek_state: OpSeekState, + seek_state: OpSeekState, } impl ProgramState { @@ -288,7 +288,7 @@ impl ProgramState { op_new_rowid_state: OpNewRowidState::Start, op_idx_insert_state: OpIdxInsertState::SeekIfUnique, op_insert_state: OpInsertState::Insert, - op_seek_state: OpSeekState::Start, + seek_state: OpSeekState::Start, } }