From e7fa023c26e2354d04288ebe90f9615560bc6f53 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 13:05:08 -0300 Subject: [PATCH 01/18] Adding indexes to the update plan --- core/translate/emitter.rs | 19 +++++++++++++++++++ core/vdbe/insn.rs | 11 ++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 6557ca169..6b1a040a0 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -668,6 +668,25 @@ fn emit_update_insns( _ => return Ok(()), }; + // dbg!(&plan.indexes); + // THIS IS SAME CODE AS WE HAVE ON INSERT + // allocate cursor id's for each btree index cursor we'll need to populate the indexes + // (idx name, root_page, idx cursor id) + let idx_cursors = plan + .indexes_to_update + .iter() + .map(|idx| { + ( + &idx.name, + idx.root_page, + program.alloc_cursor_id( + Some(idx.table_name.clone()), + CursorType::BTreeIndex(idx.clone()), + ), + ) + }) + .collect::>(); + for cond in plan .where_clause .iter() diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 587695d30..b41d48040 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -82,7 +82,7 @@ impl IdxInsertFlags { } #[derive(Clone, Copy, Debug)] -pub enum RegisterOrLiteral { +pub enum RegisterOrLiteral { Register(usize), Literal(T), } @@ -93,6 +93,15 @@ impl From for RegisterOrLiteral { } } +impl std::fmt::Display for RegisterOrLiteral { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Literal(lit) => lit.fmt(f), + Self::Register(reg) => reg.fmt(f), + } + } +} + #[derive(Description, Debug)] pub enum Insn { /// Initialize the program state and jump to the given PC. From 5bae32fe3f52500f4eec807fb5f4b6c3e9a53208 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 13:16:15 -0300 Subject: [PATCH 02/18] modified OpenWrite to include index or table name in explain --- core/translate/emitter.rs | 1 + core/translate/index.rs | 2 ++ core/translate/insert.rs | 3 +++ core/translate/main_loop.rs | 5 +++++ core/translate/schema.rs | 3 +++ core/vdbe/explain.rs | 3 ++- core/vdbe/insn.rs | 1 + 7 files changed, 17 insertions(+), 1 deletion(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 6b1a040a0..b1f98959e 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -485,6 +485,7 @@ fn emit_delete_insns( program.emit_insn(Insn::OpenWrite { cursor_id: index_cursor_id, root_page: RegisterOrLiteral::Literal(index.root_page), + name: index.name.clone(), }); let num_regs = index.columns.len() + 1; let start_reg = program.alloc_registers(num_regs); diff --git a/core/translate/index.rs b/core/translate/index.rs index 4ab8486dd..34a28db25 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -100,6 +100,7 @@ pub fn translate_create_index( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: RegisterOrLiteral::Literal(sqlite_table.root_page), + name: sqlite_table.name.clone(), }); let sql = create_idx_stmt_to_sql(&tbl_name, &idx_name, unique_if_not_exists, &columns); emit_schema_entry( @@ -181,6 +182,7 @@ pub fn translate_create_index( program.emit_insn(Insn::OpenWrite { cursor_id: btree_cursor_id, root_page: RegisterOrLiteral::Register(root_page_reg), + name: idx_name.clone() }); let sorted_loop_start = program.allocate_label(); diff --git a/core/translate/insert.rs b/core/translate/insert.rs index d162f2c2e..b6daba4c2 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -177,6 +177,7 @@ pub fn translate_insert( program.emit_insn(Insn::OpenWrite { cursor_id, root_page: RegisterOrLiteral::Literal(root_page), + name: table_name.0.clone(), }); // Main loop @@ -192,6 +193,7 @@ pub fn translate_insert( program.emit_insn(Insn::OpenWrite { cursor_id, root_page: RegisterOrLiteral::Literal(root_page), + name: table_name.0.clone(), }); populate_column_registers( @@ -209,6 +211,7 @@ pub fn translate_insert( program.emit_insn(Insn::OpenWrite { cursor_id: idx_cursor.2, root_page: idx_cursor.1.into(), + name: idx_cursor.0.clone(), }); } // Common record insertion logic for both single and multiple rows diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index bf30b8508..9e10e05b7 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -110,6 +110,7 @@ pub fn init_loop( cursor_id: table_cursor_id .expect("table cursor is always opened in OperationMode::DELETE"), root_page: root_page.into(), + name: btree.name.clone(), }); } (OperationMode::UPDATE, Table::BTree(btree)) => { @@ -118,11 +119,13 @@ pub fn init_loop( cursor_id: table_cursor_id .expect("table cursor is always opened in OperationMode::UPDATE"), root_page: root_page.into(), + name: btree.name.clone(), }); if let Some(index_cursor_id) = index_cursor_id { program.emit_insn(Insn::OpenWrite { cursor_id: index_cursor_id, root_page: index.as_ref().unwrap().root_page.into(), + name: index.as_ref().unwrap().name.clone(), }); } } @@ -148,6 +151,7 @@ pub fn init_loop( program.emit_insn(Insn::OpenWrite { cursor_id: table_cursor_id, root_page: table.table.get_root_page().into(), + name: table.table.get_name().to_string(), }); } _ => { @@ -174,6 +178,7 @@ pub fn init_loop( cursor_id: index_cursor_id .expect("index cursor is always opened in Seek with index"), root_page: index.root_page.into(), + name: index.name.clone(), }); } _ => { diff --git a/core/translate/schema.rs b/core/translate/schema.rs index 8fcb8abb8..96d4aced8 100644 --- a/core/translate/schema.rs +++ b/core/translate/schema.rs @@ -120,6 +120,7 @@ pub fn translate_create_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), + name: tbl_name.name.0.clone() }); // Add the table entry to sqlite_schema @@ -582,6 +583,7 @@ pub fn translate_create_virtual_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), + name: table_name.clone() }); let sql = create_vtable_body_to_str(&vtab, vtab_module.clone()); @@ -661,6 +663,7 @@ pub fn translate_drop_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), + name: tbl_name.name.0.clone() }); // 1. Remove all entries from the schema table related to the table we are dropping, except for triggers diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index ff5892ddb..ed16adc2c 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -1122,6 +1122,7 @@ pub fn insn_to_str( Insn::OpenWrite { cursor_id, root_page, + name, .. } => ( "OpenWrite", @@ -1133,7 +1134,7 @@ pub fn insn_to_str( 0, OwnedValue::build_text(""), 0, - "".to_string(), + format!("root={}; {}", root_page, name), ), Insn::Copy { src_reg, diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index b41d48040..5cdcee287 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -708,6 +708,7 @@ pub enum Insn { OpenWrite { cursor_id: CursorID, root_page: RegisterOrLiteral, + name: String, }, Copy { From 9aebfa7b5d5d400b7d27d29b1f7b50a4850cc440 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 14:50:41 -0300 Subject: [PATCH 03/18] open cursors for write only once --- core/translate/emitter.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index b1f98959e..19d018689 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -688,6 +688,23 @@ fn emit_update_insns( }) .collect::>(); + let open_indices_label = program.allocate_label(); + // Open all cursors Once + program.emit_insn(Insn::Once { + target_pc_when_reentered: open_indices_label, + }); + + // Open all the index btrees for writing + for idx_cursor in idx_cursors.iter() { + program.emit_insn(Insn::OpenWrite { + cursor_id: idx_cursor.2, + root_page: idx_cursor.1.into(), + name: idx_cursor.0.clone(), + }); + } + + program.resolve_label(open_indices_label, program.offset()); + for cond in plan .where_clause .iter() From 5f2216cf8eef0684f5e42ef7b5ff821e04e93967 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 15:16:16 -0300 Subject: [PATCH 04/18] modify explain for MakeRecord to show index name --- core/translate/emitter.rs | 1 + core/translate/index.rs | 3 ++- core/translate/insert.rs | 12 +++++++----- core/translate/main_loop.rs | 1 + core/translate/order_by.rs | 1 + core/translate/schema.rs | 8 +++++--- core/vdbe/execute.rs | 1 + core/vdbe/explain.rs | 33 +++++++++++++++++++-------------- core/vdbe/insn.rs | 1 + 9 files changed, 38 insertions(+), 23 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 19d018689..0a3b71fdf 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -880,6 +880,7 @@ fn emit_update_insns( start_reg: start, count: table_ref.columns().len(), dest_reg: record_reg, + index_name: None, }); program.emit_insn(Insn::Insert { cursor: cursor_id, diff --git a/core/translate/index.rs b/core/translate/index.rs index 34a28db25..f104120a0 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -165,6 +165,7 @@ pub fn translate_create_index( start_reg, count: columns.len() + 1, dest_reg: record_reg, + index_name: Some(idx_name.clone()), }); program.emit_insn(Insn::SorterInsert { cursor_id: sorter_cursor_id, @@ -182,7 +183,7 @@ pub fn translate_create_index( program.emit_insn(Insn::OpenWrite { cursor_id: btree_cursor_id, root_page: RegisterOrLiteral::Register(root_page_reg), - name: idx_name.clone() + name: idx_name.clone(), }); let sorted_loop_start = program.allocate_label(); diff --git a/core/translate/insert.rs b/core/translate/insert.rs index b6daba4c2..1c3169b4c 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -292,7 +292,7 @@ pub fn translate_insert( _ => (), } - for index_col_mapping in index_col_mappings.iter() { + for index_col_mapping in index_col_mappings { // find which cursor we opened earlier for this index let idx_cursor_id = idx_cursors .iter() @@ -321,17 +321,18 @@ pub fn translate_insert( amount: 0, }); + let index = schema + .get_index(&table_name.0, &index_col_mapping.idx_name) + .expect("index should be present"); + let record_reg = program.alloc_register(); program.emit_insn(Insn::MakeRecord { start_reg: idx_start_reg, count: num_cols + 1, dest_reg: record_reg, + index_name: Some(index_col_mapping.idx_name), }); - let index = schema - .get_index(&table_name.0, &index_col_mapping.idx_name) - .expect("index should be present"); - if index.unique { let label_idx_insert = program.allocate_label(); program.emit_insn(Insn::NoConflict { @@ -387,6 +388,7 @@ pub fn translate_insert( start_reg: column_registers_start, count: num_cols, dest_reg: record_register, + index_name: None, }); program.emit_insn(Insn::Insert { diff --git a/core/translate/main_loop.rs b/core/translate/main_loop.rs index 9e10e05b7..f10523f16 100644 --- a/core/translate/main_loop.rs +++ b/core/translate/main_loop.rs @@ -1227,6 +1227,7 @@ fn emit_autoindex( start_reg: ephemeral_cols_start_reg, count: num_regs_to_reserve, dest_reg: record_reg, + index_name: Some(index.name.clone()), }); program.emit_insn(Insn::IdxInsert { cursor_id: index_cursor_id, diff --git a/core/translate/order_by.rs b/core/translate/order_by.rs index 6c8f4d90a..488a8cd82 100644 --- a/core/translate/order_by.rs +++ b/core/translate/order_by.rs @@ -247,6 +247,7 @@ pub fn sorter_insert( start_reg, count: column_count, dest_reg: record_reg, + index_name: None, }); program.emit_insn(Insn::SorterInsert { cursor_id, diff --git a/core/translate/schema.rs b/core/translate/schema.rs index 96d4aced8..7c0b3d7cc 100644 --- a/core/translate/schema.rs +++ b/core/translate/schema.rs @@ -120,7 +120,7 @@ pub fn translate_create_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), - name: tbl_name.name.0.clone() + name: tbl_name.name.0.clone(), }); // Add the table entry to sqlite_schema @@ -237,6 +237,7 @@ pub fn emit_schema_entry( start_reg: type_reg, count: 5, dest_reg: record_reg, + index_name: None, }); program.emit_insn(Insn::Insert { @@ -564,6 +565,7 @@ pub fn translate_create_virtual_table( start_reg: args_start, count: args_vec.len(), dest_reg: args_record_reg, + index_name: None, }); Some(args_record_reg) } else { @@ -583,7 +585,7 @@ pub fn translate_create_virtual_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), - name: table_name.clone() + name: table_name.clone(), }); let sql = create_vtable_body_to_str(&vtab, vtab_module.clone()); @@ -663,7 +665,7 @@ pub fn translate_drop_table( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: 1usize.into(), - name: tbl_name.name.0.clone() + name: tbl_name.name.0.clone(), }); // 1. Remove all entries from the schema table related to the table we are dropping, except for triggers diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index d6ab6ba4d..62e953813 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -1444,6 +1444,7 @@ pub fn op_make_record( start_reg, count, dest_reg, + .. } = insn else { unreachable!("unexpected Insn {:?}", insn) diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index ed16adc2c..28e48f2cc 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -529,20 +529,25 @@ pub fn insn_to_str( start_reg, count, dest_reg, - } => ( - "MakeRecord", - *start_reg as i32, - *count as i32, - *dest_reg as i32, - OwnedValue::build_text(""), - 0, - format!( - "r[{}]=mkrec(r[{}..{}])", - dest_reg, - start_reg, - start_reg + count - 1, - ), - ), + index_name, + } => { + let for_index = index_name.as_ref().map(|name| format!(" ;for {}", name)); + ( + "MakeRecord", + *start_reg as i32, + *count as i32, + *dest_reg as i32, + OwnedValue::build_text(""), + 0, + format!( + "r[{}]=mkrec(r[{}..{}]){}", + dest_reg, + start_reg, + start_reg + count - 1, + for_index.unwrap_or("".to_string()) + ), + ) + } Insn::ResultRow { start_reg, count } => ( "ResultRow", *start_reg as i32, diff --git a/core/vdbe/insn.rs b/core/vdbe/insn.rs index 5cdcee287..876ab7930 100644 --- a/core/vdbe/insn.rs +++ b/core/vdbe/insn.rs @@ -373,6 +373,7 @@ pub enum Insn { start_reg: usize, // P1 count: usize, // P2 dest_reg: usize, // P3 + index_name: Option, }, /// Emit a row of results. From 60a99851f86304d073e7a869d2e2d1eb56834b65 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 16:50:02 -0300 Subject: [PATCH 05/18] emit NoConflict and Halt. Already detects unique constraints --- core/translate/emitter.rs | 65 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 0a3b71fdf..a7ff2935f 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use limbo_sqlite3_parser::ast::{self}; +use crate::error::SQLITE_CONSTRAINT_PRIMARYKEY; use crate::function::Func; use crate::schema::Index; use crate::translate::plan::{DeletePlan, Plan, Search}; @@ -758,6 +759,69 @@ fn emit_update_insns( }); } + for (pos, index) in plan.indexes.iter().enumerate() { + if index.unique { + let (idx_name, idx_root_page, idx_cursor_id) = + idx_cursors.get(pos).expect("index cursor should exist"); + + let num_cols = index.columns.len(); + // allocate scratch registers for the index columns plus rowid + let idx_start_reg = program.alloc_registers(num_cols + 1); + + let rowid_reg = beg; + let idx_cols_start_reg = beg + 1; + + // copy each index column from the table's column registers into these scratch regs + for (i, col) in index.columns.iter().enumerate() { + // copy from the table's column register over to the index's scratch register + + program.emit_insn(Insn::Copy { + src_reg: idx_cols_start_reg + i, + dst_reg: idx_start_reg + i, + amount: 0, + }); + } + // last register is the rowid + program.emit_insn(Insn::Copy { + src_reg: rowid_reg, + dst_reg: idx_start_reg + num_cols, + amount: 0, + }); + + let constraint_check = program.allocate_label(); + program.emit_insn(Insn::NoConflict { + cursor_id: *idx_cursor_id, + target_pc: constraint_check, + record_reg: idx_start_reg, + num_regs: num_cols, + }); + + let column_names = index.columns.iter().enumerate().fold( + String::with_capacity(50), + |mut accum, (idx, col)| { + if idx > 0 { + accum.push_str(", "); + } + accum.push_str(&table_ref.table.get_name()); + accum.push('.'); + accum.push_str(&col.name); + + accum + }, + ); + + // TODO: Emit IdxRowId to get the rowId of the last entry the index points to + // TODO: Emit Eq to see if rowId that index points to matches the rowId of current row + + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index + description: column_names, + }); + + program.resolve_label(constraint_check, program.offset()); + } + } + for cond in plan .where_clause .iter() @@ -882,6 +946,7 @@ fn emit_update_insns( dest_reg: record_reg, index_name: None, }); + // TODO: Delete opcode should be emitted here program.emit_insn(Insn::Insert { cursor: cursor_id, key_reg: beg, From 6457d7675aa7c977e3a1a0ec17c91e802723e277 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 18:09:19 -0300 Subject: [PATCH 06/18] instruction emitted should be correct, but having an infinite loop bug --- core/translate/emitter.rs | 78 +++++++++++++++++++++++++++++++++++---- core/vdbe/explain.rs | 2 +- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index a7ff2935f..a7fbcbd57 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -12,7 +12,7 @@ use crate::schema::Index; use crate::translate::plan::{DeletePlan, Plan, Search}; use crate::util::exprs_are_equivalent; use crate::vdbe::builder::{CursorType, ProgramBuilder}; -use crate::vdbe::insn::{IdxInsertFlags, RegisterOrLiteral}; +use crate::vdbe::insn::{CmpInsFlags, IdxInsertFlags, RegisterOrLiteral}; use crate::vdbe::{insn::Insn, BranchOffset}; use crate::{Result, SymbolTable}; @@ -670,7 +670,6 @@ fn emit_update_insns( _ => return Ok(()), }; - // dbg!(&plan.indexes); // THIS IS SAME CODE AS WE HAVE ON INSERT // allocate cursor id's for each btree index cursor we'll need to populate the indexes // (idx name, root_page, idx cursor id) @@ -678,6 +677,7 @@ fn emit_update_insns( .indexes_to_update .iter() .map(|idx| { + let record_reg = program.alloc_register(); ( &idx.name, idx.root_page, @@ -685,9 +685,10 @@ fn emit_update_insns( Some(idx.table_name.clone()), CursorType::BTreeIndex(idx.clone()), ), + record_reg, ) }) - .collect::>(); + .collect::>(); let open_indices_label = program.allocate_label(); // Open all cursors Once @@ -759,9 +760,9 @@ fn emit_update_insns( }); } - for (pos, index) in plan.indexes.iter().enumerate() { + for (pos, index) in plan.indexes_to_update.iter().enumerate() { if index.unique { - let (idx_name, idx_root_page, idx_cursor_id) = + let (_, _, idx_cursor_id, record_reg) = idx_cursors.get(pos).expect("index cursor should exist"); let num_cols = index.columns.len(); @@ -772,7 +773,7 @@ fn emit_update_insns( let idx_cols_start_reg = beg + 1; // copy each index column from the table's column registers into these scratch regs - for (i, col) in index.columns.iter().enumerate() { + for i in 0..num_cols { // copy from the table's column register over to the index's scratch register program.emit_insn(Insn::Copy { @@ -788,6 +789,13 @@ fn emit_update_insns( amount: 0, }); + program.emit_insn(Insn::MakeRecord { + start_reg: idx_start_reg, + count: num_cols + 1, + dest_reg: *record_reg, + index_name: Some(index.name.clone()), + }); + let constraint_check = program.allocate_label(); program.emit_insn(Insn::NoConflict { cursor_id: *idx_cursor_id, @@ -810,8 +818,18 @@ fn emit_update_insns( }, ); - // TODO: Emit IdxRowId to get the rowId of the last entry the index points to - // TODO: Emit Eq to see if rowId that index points to matches the rowId of current row + let idx_rowid_reg = program.alloc_register(); + program.emit_insn(Insn::IdxRowId { + cursor_id: *idx_cursor_id, + dest: idx_rowid_reg, + }); + + program.emit_insn(Insn::Eq { + lhs: rowid_reg, + rhs: idx_rowid_reg, + target_pc: constraint_check, + flags: CmpInsFlags::default(), // TODO: not sure type of comparison flag is needed + }); program.emit_insn(Insn::Halt { err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index @@ -946,6 +964,50 @@ fn emit_update_insns( dest_reg: record_reg, index_name: None, }); + + // For each index -> insert + for (pos, index) in plan.indexes_to_update.iter().enumerate() { + let num_regs = index.columns.len() + 1; + let start_reg = program.alloc_registers(num_regs); + + // Emit columns that are part of the index + index + .columns + .iter() + .enumerate() + .for_each(|(reg_offset, column_index)| { + program.emit_insn(Insn::Column { + cursor_id, + column: column_index.pos_in_table, + dest: start_reg + reg_offset, + }); + }); + + program.emit_insn(Insn::RowId { + cursor_id, + dest: start_reg + num_regs - 1, + }); + + let (_, _, idx_cursor_id, record_reg) = + idx_cursors.get(pos).expect("index cursor should exist"); + + program.emit_insn(Insn::IdxDelete { + start_reg, + num_regs, + cursor_id: *idx_cursor_id, + }); + + program.emit_insn(Insn::IdxInsert { + cursor_id: *idx_cursor_id, + record_reg: *record_reg, + unpacked_start: Some(start), + unpacked_count: Some((index.columns.len() + 1) as u16), + flags: IdxInsertFlags::new(), + }); + } + + program.emit_insn(Insn::Delete { cursor_id }); + // TODO: Delete opcode should be emitted here program.emit_insn(Insn::Insert { cursor: cursor_id, diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index 28e48f2cc..e33b8eb88 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -531,7 +531,7 @@ pub fn insn_to_str( dest_reg, index_name, } => { - let for_index = index_name.as_ref().map(|name| format!(" ;for {}", name)); + let for_index = index_name.as_ref().map(|name| format!("; for {}", name)); ( "MakeRecord", *start_reg as i32, From cf7f60b8f531d71e7ebbc5a9297d7cd7a0d2f2b5 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 21:15:03 -0300 Subject: [PATCH 07/18] changed from resolve_label to preassign_label --- core/translate/emitter.rs | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index a7fbcbd57..eaf355417 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -690,22 +690,25 @@ fn emit_update_insns( }) .collect::>(); - let open_indices_label = program.allocate_label(); - // Open all cursors Once - program.emit_insn(Insn::Once { - target_pc_when_reentered: open_indices_label, - }); + if !idx_cursors.is_empty() { + let open_indices_label = program.allocate_label(); - // Open all the index btrees for writing - for idx_cursor in idx_cursors.iter() { - program.emit_insn(Insn::OpenWrite { - cursor_id: idx_cursor.2, - root_page: idx_cursor.1.into(), - name: idx_cursor.0.clone(), + // Open all cursors Once + program.emit_insn(Insn::Once { + target_pc_when_reentered: open_indices_label, }); - } - program.resolve_label(open_indices_label, program.offset()); + // Open all the index btrees for writing + for idx_cursor in idx_cursors.iter() { + program.emit_insn(Insn::OpenWrite { + cursor_id: idx_cursor.2, + root_page: idx_cursor.1.into(), + name: idx_cursor.0.clone(), + }); + } + + program.preassign_label_to_next_insn(open_indices_label); + } for cond in plan .where_clause @@ -828,15 +831,15 @@ fn emit_update_insns( lhs: rowid_reg, rhs: idx_rowid_reg, target_pc: constraint_check, - flags: CmpInsFlags::default(), // TODO: not sure type of comparison flag is needed + flags: CmpInsFlags::default(), // TODO: not sure what type of comparison flag is needed }); program.emit_insn(Insn::Halt { - err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index for error code description: column_names, }); - program.resolve_label(constraint_check, program.offset()); + program.preassign_label_to_next_insn(constraint_check); } } @@ -1006,9 +1009,6 @@ fn emit_update_insns( }); } - program.emit_insn(Insn::Delete { cursor_id }); - - // TODO: Delete opcode should be emitted here program.emit_insn(Insn::Insert { cursor: cursor_id, key_reg: beg, From 3aaf4206b7e5b0d2495a1650e15398cb9ec01d55 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 29 Apr 2025 22:48:57 -0300 Subject: [PATCH 08/18] altered constraint tests to create bad update statements. Tests caught a bug where I was copying the wrong values from the registers --- core/translate/emitter.rs | 5 ++-- core/vdbe/explain.rs | 25 ++++++++++++-------- testing/cli_tests/constraint.py | 41 +++++++++++++++++++++++++++++++-- 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index eaf355417..f5243c9c5 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -775,12 +775,13 @@ fn emit_update_insns( let rowid_reg = beg; let idx_cols_start_reg = beg + 1; + // copy each index column from the table's column registers into these scratch regs - for i in 0..num_cols { + for (i, col) in index.columns.iter().enumerate(){ // copy from the table's column register over to the index's scratch register program.emit_insn(Insn::Copy { - src_reg: idx_cols_start_reg + i, + src_reg: idx_cols_start_reg + col.pos_in_table, dst_reg: idx_start_reg + i, amount: 0, }); diff --git a/core/vdbe/explain.rs b/core/vdbe/explain.rs index e33b8eb88..7ac6e397d 100644 --- a/core/vdbe/explain.rs +++ b/core/vdbe/explain.rs @@ -1086,15 +1086,22 @@ pub fn insn_to_str( target_pc, record_reg, num_regs, - } => ( - "NoConflict", - *cursor_id as i32, - target_pc.to_debug_int(), - *record_reg as i32, - OwnedValue::build_text(&format!("{num_regs}")), - 0, - format!("key=r[{}]", record_reg), - ), + } => { + let key = if *num_regs > 0 { + format!("key=r[{}..{}]", record_reg, record_reg + num_regs - 1) + } else { + format!("key=r[{}]", record_reg) + }; + ( + "NoConflict", + *cursor_id as i32, + target_pc.to_debug_int(), + *record_reg as i32, + OwnedValue::build_text(&format!("{num_regs}")), + 0, + key, + ) + } Insn::NotExists { cursor, rowid_reg, diff --git a/testing/cli_tests/constraint.py b/testing/cli_tests/constraint.py index 65758745b..9d5003840 100644 --- a/testing/cli_tests/constraint.py +++ b/testing/cli_tests/constraint.py @@ -230,12 +230,24 @@ class Table(BaseModel): return f"INSERT INTO {self.name} VALUES ({vals});" + # These statements should always cause a constraint error as there is no where clause here + def generate_update(self) -> str: + vals = [ + f"{col.name} = {col.col_type.generate(fake)}" + for col in self.columns + if col.primary_key + ] + vals = ", ".join(vals) + + return f"UPDATE {self.name} SET {vals};" + class ConstraintTest(BaseModel): table: Table db_path: str = "testing/constraint.db" insert_stmts: list[str] insert_errors: list[str] + update_errors: list[str] def run( self, @@ -258,6 +270,14 @@ class ConstraintTest(BaseModel): str(len(self.insert_stmts)), ) + for update_stmt in self.update_errors: + limbo.run_test_fn( + update_stmt, + lambda val: "Runtime error: UNIQUE constraint failed" in val, + ) + + # TODO: When we implement rollbacks, have a test here to assure the values did not change + def validate_with_expected(result: str, expected: str): return (expected in result, expected) @@ -281,8 +301,18 @@ def generate_test(col_amount: int, primary_keys: int) -> ConstraintTest: table = Table(columns=cols, name=fake.word()) insert_stmts = [table.generate_insert() for _ in range(col_amount)] + + update_errors = [] + if len(insert_stmts) > 1: + update_errors = [ + table.generate_update() for _ in table.columns if col.primary_key + ] + return ConstraintTest( - table=table, insert_stmts=insert_stmts, insert_errors=insert_stmts + table=table, + insert_stmts=insert_stmts, + insert_errors=insert_stmts, + update_errors=update_errors, ) @@ -296,8 +326,15 @@ def custom_test_1() -> ConstraintTest: "INSERT INTO users VALUES (1, 'alice');", "INSERT INTO users VALUES (2, 'bob');", ] + update_stmts = [ + "UPDATE users SET id = 3;", + "UPDATE users SET id = 2, username = 'bob' WHERE id == 1;", + ] return ConstraintTest( - table=table, insert_stmts=insert_stmts, insert_errors=insert_stmts + table=table, + insert_stmts=insert_stmts, + insert_errors=insert_stmts, + update_errors=update_stmts, ) From 758dfff2fe590e47c39d52c97df2c66b8e93949b Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Wed, 30 Apr 2025 14:26:50 -0300 Subject: [PATCH 09/18] modified tests as we do not have rollback yet. Also correctly raise a contraint error on primary keys only --- core/translate/emitter.rs | 128 +++++++++++++++++++++++++++----- testing/cli_tests/constraint.py | 6 +- 2 files changed, 113 insertions(+), 21 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index f5243c9c5..c378885f7 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -742,11 +742,45 @@ fn emit_update_insns( cursor_id, dest: beg, }); - // if no rowid, we're done - program.emit_insn(Insn::IsNull { - reg: beg, - target_pc: t_ctx.label_main_loop_end.unwrap(), - }); + + // Check if rowid was provided (through INTEGER PRIMARY KEY as a rowid alias) + let rowid_alias_index = table_ref.columns().iter().position(|c| c.is_rowid_alias); + let rowid_set_clause_reg = if rowid_alias_index.is_some() { + Some(program.alloc_register()) + } else { + None + }; + let has_user_provided_rowid = { + if let Some(index) = rowid_alias_index { + plan.set_clauses + .iter() + .position(|(idx, _)| *idx == index) + .is_some() + } else { + false + } + }; + + let check_rowid_not_exists_label = if has_user_provided_rowid { + Some(program.allocate_label()) + } else { + None + }; + + if has_user_provided_rowid { + program.emit_insn(Insn::NotExists { + cursor: cursor_id, + rowid_reg: beg, + target_pc: check_rowid_not_exists_label.unwrap(), + }); + } else { + // if no rowid, we're done + program.emit_insn(Insn::IsNull { + reg: beg, + target_pc: t_ctx.label_main_loop_end.unwrap(), + }); + } + if is_virtual { program.emit_insn(Insn::Copy { src_reg: beg, @@ -775,9 +809,8 @@ fn emit_update_insns( let rowid_reg = beg; let idx_cols_start_reg = beg + 1; - // copy each index column from the table's column registers into these scratch regs - for (i, col) in index.columns.iter().enumerate(){ + for (i, col) in index.columns.iter().enumerate() { // copy from the table's column register over to the index's scratch register program.emit_insn(Insn::Copy { @@ -906,18 +939,38 @@ fn emit_update_insns( // we scan a column at a time, loading either the column's values, or the new value // from the Set expression, into registers so we can emit a MakeRecord and update the row. let start = if is_virtual { beg + 2 } else { beg + 1 }; - for idx in 0..table_ref.columns().len() { + for (idx, table_column) in table_ref.columns().iter().enumerate() { let target_reg = start + idx; if let Some((_, expr)) = plan.set_clauses.iter().find(|(i, _)| *i == idx) { - translate_expr( - program, - Some(&plan.table_references), - expr, - target_reg, - &t_ctx.resolver, - )?; + if has_user_provided_rowid + && (table_column.primary_key || table_column.is_rowid_alias) + && !is_virtual + { + let rowid_set_clause_reg = rowid_set_clause_reg.unwrap(); + translate_expr( + program, + Some(&plan.table_references), + expr, + rowid_set_clause_reg, + &t_ctx.resolver, + )?; + + program.emit_insn(Insn::MustBeInt { + reg: rowid_set_clause_reg, + }); + + program.emit_null(target_reg, None); + } else { + translate_expr( + program, + Some(&plan.table_references), + expr, + target_reg, + &t_ctx.resolver, + )?; + } + // if let Some(rowid_reg) = rowid_set_clause_reg {} } else { - let table_column = table_ref.table.columns().get(idx).unwrap(); let column_idx_in_index = index.as_ref().and_then(|(idx, _)| { idx.columns .iter() @@ -961,6 +1014,42 @@ fn emit_update_insns( table_reference: Rc::clone(&btree_table), }); } + + if has_user_provided_rowid { + let record_label = program.allocate_label(); + let idx = rowid_alias_index.unwrap(); + let target_reg = rowid_set_clause_reg.unwrap(); + program.emit_insn(Insn::Eq { + lhs: target_reg, + rhs: beg, + target_pc: record_label, + flags: CmpInsFlags::default(), + }); + + program.emit_insn(Insn::NotExists { + cursor: cursor_id, + rowid_reg: target_reg, + target_pc: record_label, + }); + + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, + description: format!( + "{}.{}", + table_ref.table.get_name(), + &table_ref + .columns() + .get(idx) + .unwrap() + .name + .as_ref() + .map_or("", |v| v) + ), + }); + + program.preassign_label_to_next_insn(record_label); + } + let record_reg = program.alloc_register(); program.emit_insn(Insn::MakeRecord { start_reg: start, @@ -1012,7 +1101,7 @@ fn emit_update_insns( program.emit_insn(Insn::Insert { cursor: cursor_id, - key_reg: beg, + key_reg: rowid_set_clause_reg.unwrap_or(beg), record_reg, flag: 0, table_name: table_ref.identifier.clone(), @@ -1035,5 +1124,10 @@ fn emit_update_insns( }) } // TODO(pthorpe): handle RETURNING clause + + if let Some(label) = check_rowid_not_exists_label { + program.preassign_label_to_next_insn(label); + } + Ok(()) } diff --git a/testing/cli_tests/constraint.py b/testing/cli_tests/constraint.py index 9d5003840..36181fa67 100644 --- a/testing/cli_tests/constraint.py +++ b/testing/cli_tests/constraint.py @@ -304,9 +304,8 @@ def generate_test(col_amount: int, primary_keys: int) -> ConstraintTest: update_errors = [] if len(insert_stmts) > 1: - update_errors = [ - table.generate_update() for _ in table.columns if col.primary_key - ] + # TODO: As we have no rollback we just generate one update statement + update_errors = [table.generate_update()] return ConstraintTest( table=table, @@ -327,7 +326,6 @@ def custom_test_1() -> ConstraintTest: "INSERT INTO users VALUES (2, 'bob');", ] update_stmts = [ - "UPDATE users SET id = 3;", "UPDATE users SET id = 2, username = 'bob' WHERE id == 1;", ] return ConstraintTest( From 482634b598d4bcc0e96155790a1f505ebe7880f1 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Thu, 1 May 2025 00:08:40 -0300 Subject: [PATCH 10/18] adjust null opcode emission based in rowid_alias --- core/translate/emitter.rs | 92 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index c378885f7..6aacff3c1 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -744,6 +744,7 @@ fn emit_update_insns( }); // Check if rowid was provided (through INTEGER PRIMARY KEY as a rowid alias) + let rowid_alias_index = table_ref.columns().iter().position(|c| c.is_rowid_alias); let rowid_set_clause_reg = if rowid_alias_index.is_some() { Some(program.alloc_register()) @@ -1005,6 +1006,87 @@ fn emit_update_insns( } } } + + for (pos, index) in plan.indexes.iter().enumerate() { + if index.unique { + let (_, _, idx_cursor_id, record_reg) = + idx_cursors.get(pos).expect("index cursor should exist"); + + let num_cols = index.columns.len(); + // allocate scratch registers for the index columns plus rowid + let idx_start_reg = program.alloc_registers(num_cols + 1); + + let rowid_reg = beg; + let idx_cols_start_reg = beg + 1; + + // copy each index column from the table's column registers into these scratch regs + for (i, col) in index.columns.iter().enumerate() { + // copy from the table's column register over to the index's scratch register + + program.emit_insn(Insn::Copy { + src_reg: idx_cols_start_reg + col.pos_in_table, + dst_reg: idx_start_reg + i, + amount: 0, + }); + } + // last register is the rowid + program.emit_insn(Insn::Copy { + src_reg: rowid_reg, + dst_reg: idx_start_reg + num_cols, + amount: 0, + }); + + program.emit_insn(Insn::MakeRecord { + start_reg: idx_start_reg, + count: num_cols + 1, + dest_reg: *record_reg, + index_name: Some(index.name.clone()), + }); + + let constraint_check = program.allocate_label(); + program.emit_insn(Insn::NoConflict { + cursor_id: *idx_cursor_id, + target_pc: constraint_check, + record_reg: idx_start_reg, + num_regs: num_cols, + }); + + let column_names = index.columns.iter().enumerate().fold( + String::with_capacity(50), + |mut accum, (idx, col)| { + if idx > 0 { + accum.push_str(", "); + } + accum.push_str(&table_ref.table.get_name()); + accum.push('.'); + accum.push_str(&col.name); + + accum + }, + ); + + let idx_rowid_reg = program.alloc_register(); + program.emit_insn(Insn::IdxRowId { + cursor_id: *idx_cursor_id, + dest: idx_rowid_reg, + }); + + program.emit_insn(Insn::Eq { + lhs: rowid_reg, + rhs: idx_rowid_reg, + target_pc: constraint_check, + flags: CmpInsFlags::default(), // TODO: not sure what type of comparison flag is needed + }); + + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index for error code + description: column_names, + }); + + program.preassign_label_to_next_insn(constraint_check); + } + } + if let Some(btree_table) = table_ref.btree() { if btree_table.is_strict { program.emit_insn(Insn::TypeCheck { @@ -1058,6 +1140,14 @@ fn emit_update_insns( index_name: None, }); + if has_user_provided_rowid { + program.emit_insn(Insn::NotExists { + cursor: cursor_id, + rowid_reg: beg, + target_pc: check_rowid_not_exists_label.unwrap(), + }); + } + // For each index -> insert for (pos, index) in plan.indexes_to_update.iter().enumerate() { let num_regs = index.columns.len() + 1; @@ -1099,6 +1189,8 @@ fn emit_update_insns( }); } + // program.emit_insn(Insn::Delete { cursor_id }); + program.emit_insn(Insn::Insert { cursor: cursor_id, key_reg: rowid_set_clause_reg.unwrap_or(beg), From 6588004f809208fb5f8439e6d81e15079a18ce9a Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Thu, 1 May 2025 01:37:34 -0300 Subject: [PATCH 11/18] fix incorrectly detecting if user provided row_id_alias to set clause --- core/translate/emitter.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 6aacff3c1..a19d2cfcc 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -745,22 +745,20 @@ fn emit_update_insns( // Check if rowid was provided (through INTEGER PRIMARY KEY as a rowid alias) - let rowid_alias_index = table_ref.columns().iter().position(|c| c.is_rowid_alias); + let rowid_alias_index = { + let rowid_alias_index = table_ref.columns().iter().position(|c| c.is_rowid_alias); + if let Some(index) = rowid_alias_index { + plan.set_clauses.iter().position(|(idx, _)| *idx == index) + } else { + None + } + }; let rowid_set_clause_reg = if rowid_alias_index.is_some() { Some(program.alloc_register()) } else { None }; - let has_user_provided_rowid = { - if let Some(index) = rowid_alias_index { - plan.set_clauses - .iter() - .position(|(idx, _)| *idx == index) - .is_some() - } else { - false - } - }; + let has_user_provided_rowid = rowid_alias_index.is_some(); let check_rowid_not_exists_label = if has_user_provided_rowid { Some(program.allocate_label()) From c14687734416ad328945aaa9dc95daf9d2019c1d Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Thu, 1 May 2025 16:16:59 -0300 Subject: [PATCH 12/18] add sqlite debug cli for nix. Fix cursor delete panic. Add tracing for cell indices in btree --- core/storage/btree.rs | 49 ++++++++++++++++++++++++++++++++++++++----- flake.nix | 13 +++++++++--- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index bc7c4afe2..0c306bba7 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -756,8 +756,11 @@ impl BTreeCursor { let mem_page = mem_page_rc.get(); let contents = mem_page.contents.as_ref().unwrap(); + let cell_count = contents.cell_count(); - if cell_idx == contents.cell_count() { + tracing::trace!(cell_idx, cell_count, "get_next_record"); + + if cell_count == 0 || cell_idx == cell_count { // do rightmost let has_parent = self.stack.has_parent(); match contents.rightmost_pointer() { @@ -3590,7 +3593,9 @@ impl BTreeCursor { DeleteState::FindCell => { let page = self.stack.top(); let mut cell_idx = self.stack.current_cell_index() as usize; - cell_idx -= 1; + if cell_idx > 0 { + cell_idx -= 1; + } let contents = page.get().contents.as_ref().unwrap(); if cell_idx >= contents.cell_count() { @@ -4439,6 +4444,11 @@ impl PageStack { /// Cell index of the current page fn current_cell_index(&self) -> i32 { let current = self.current(); + tracing::event!( + tracing::Level::TRACE, + "current: cell_indices={:?}", + self.cell_indices + ); self.cell_indices.borrow()[current] } @@ -4453,14 +4463,32 @@ impl PageStack { /// We usually advance after going traversing a new page fn advance(&self) { let current = self.current(); - tracing::trace!("pagestack::advance {}", self.cell_indices.borrow()[current],); + tracing::trace!( + "pagestack::advance {}, cell_indices={:?}", + self.cell_indices.borrow()[current], + self.cell_indices + ); self.cell_indices.borrow_mut()[current] += 1; + tracing::event!( + tracing::Level::TRACE, + "after: cell_indices={:?}", + self.cell_indices + ); } fn retreat(&self) { let current = self.current(); - tracing::trace!("pagestack::retreat {}", self.cell_indices.borrow()[current]); + tracing::trace!( + "pagestack::retreat {}, cell_indices={:?}", + self.cell_indices.borrow()[current], + self.cell_indices + ); self.cell_indices.borrow_mut()[current] -= 1; + tracing::event!( + tracing::Level::TRACE, + "after: cell_indices={:?}", + self.cell_indices + ); } /// Move the cursor to the next cell in the current page according to the iteration direction. @@ -4477,7 +4505,18 @@ impl PageStack { fn set_cell_index(&self, idx: i32) { let current = self.current(); - self.cell_indices.borrow_mut()[current] = idx + tracing::event!( + tracing::Level::TRACE, + "set_cell_index={} cell_indices={:?}", + idx, + self.cell_indices + ); + self.cell_indices.borrow_mut()[current] = idx; + tracing::event!( + tracing::Level::TRACE, + "after: cell_indices={:?}", + self.cell_indices + ); } fn has_parent(&self) -> bool { diff --git a/flake.nix b/flake.nix index 31f19412d..e262c09c4 100644 --- a/flake.nix +++ b/flake.nix @@ -24,10 +24,17 @@ lib = pkgs.lib; + # Custom SQLite package with debug enabled + sqlite-debug = pkgs.sqlite.overrideAttrs (oldAttrs: rec { + name = "sqlite-debug-${oldAttrs.version}"; + configureFlags = oldAttrs.configureFlags ++ [ "--enable-debug" ]; + dontStrip = true; + separateDebugInfo = true; + }); + cargoArtifacts = craneLib.buildDepsOnly { src = ./.; pname = "limbo"; - stritcDeps = true; nativeBuildInputs = with pkgs; [ python3 ]; }; @@ -58,7 +65,7 @@ devShells.default = with pkgs; mkShell { nativeBuildInputs = [ clang - sqlite + sqlite-debug # Use debug-enabled SQLite gnumake tcl python3 @@ -77,4 +84,4 @@ }; } ); -} +} \ No newline at end of file From 05f4ca28cc20cf44ebb6264a1361e5323ced21a6 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Thu, 1 May 2025 23:08:57 -0300 Subject: [PATCH 13/18] btree rewind and next fix. Keep track of rowids seen to avoid infinite loop --- core/storage/btree.rs | 19 ++++++++++++++++++- core/translate/emitter.rs | 3 +-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 0c306bba7..181c19c8e 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -20,7 +20,6 @@ use crate::{ LimboError, Result, }; -#[cfg(debug_assertions)] use std::collections::HashSet; use std::{ cell::{Cell, Ref, RefCell}, @@ -367,6 +366,7 @@ pub struct BTreeCursor { pub index_key_sort_order: IndexKeySortOrder, /// Maintain count of the number of records in the btree. Used for the `Count` opcode count: usize, + rowid_seen: HashSet, } impl BTreeCursor { @@ -393,6 +393,7 @@ impl BTreeCursor { empty_record: Cell::new(true), index_key_sort_order: IndexKeySortOrder::default(), count: 0, + rowid_seen: HashSet::new(), } } @@ -834,6 +835,11 @@ impl BTreeCursor { )? }; self.stack.advance(); + if self.rowid_seen.contains(_rowid) { + continue; + } else { + self.rowid_seen.insert(*_rowid); + } return Ok(CursorResult::Ok(Some(*_rowid))); } BTreeCell::IndexInteriorCell(IndexInteriorCell { @@ -899,6 +905,11 @@ impl BTreeCursor { Some(RefValue::Integer(rowid)) => *rowid as u64, _ => unreachable!("index cells should have an integer rowid"), }; + if self.rowid_seen.contains(&rowid) { + continue; + } else { + self.rowid_seen.insert(rowid); + } return Ok(CursorResult::Ok(Some(rowid))); } else { continue; @@ -959,6 +970,11 @@ impl BTreeCursor { Some(RefValue::Integer(rowid)) => *rowid as u64, _ => unreachable!("index cells should have an integer rowid"), }; + if self.rowid_seen.contains(&rowid) { + continue; + } else { + self.rowid_seen.insert(rowid); + } return Ok(CursorResult::Ok(Some(rowid))); } else { continue; @@ -3424,6 +3440,7 @@ impl BTreeCursor { } pub fn rewind(&mut self) -> Result> { + self.rowid_seen.clear(); if self.mv_cursor.is_some() { let rowid = return_if_io!(self.get_next_record(None)); self.rowid.replace(rowid); diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index a19d2cfcc..81a07e92e 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -968,7 +968,6 @@ fn emit_update_insns( &t_ctx.resolver, )?; } - // if let Some(rowid_reg) = rowid_set_clause_reg {} } else { let column_idx_in_index = index.as_ref().and_then(|(idx, _)| { idx.columns @@ -1187,7 +1186,7 @@ fn emit_update_insns( }); } - // program.emit_insn(Insn::Delete { cursor_id }); + program.emit_insn(Insn::Delete { cursor_id }); program.emit_insn(Insn::Insert { cursor: cursor_id, From c69f503eacf9e54239a5db85719888b4aaefd1e2 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Sun, 4 May 2025 14:52:02 -0300 Subject: [PATCH 14/18] rebase adjustments --- core/translate/emitter.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 81a07e92e..ec6eb1c7f 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -601,6 +601,7 @@ fn emit_program_for_update( program.emit_insn(Insn::OpenWrite { cursor_id: index_cursor, root_page: RegisterOrLiteral::Literal(index.root_page), + name: index.name.clone(), }); index_cursors.push(index_cursor); } @@ -926,6 +927,7 @@ fn emit_update_insns( start_reg: index_record_reg_start, count: index_record_reg_count, dest_reg: index_record_reg, + index_name: Some(index.name.clone()), }); program.emit_insn(Insn::IdxInsert { cursor_id: index_cursor, @@ -1004,7 +1006,7 @@ fn emit_update_insns( } } - for (pos, index) in plan.indexes.iter().enumerate() { + for (pos, index) in plan.indexes_to_update.iter().enumerate() { if index.unique { let (_, _, idx_cursor_id, record_reg) = idx_cursors.get(pos).expect("index cursor should exist"); From 814508981cf64d14e3365ba4c99cdbdc6f64fd14 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Mon, 5 May 2025 00:42:12 -0300 Subject: [PATCH 15/18] fixing more rebase issues and cleaning up code. Save cursor context when calling delete for later use when needed --- core/storage/btree.rs | 98 ++++++++++---------- core/translate/emitter.rs | 184 +++----------------------------------- 2 files changed, 62 insertions(+), 220 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 181c19c8e..eb6c996d1 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -20,6 +20,7 @@ use crate::{ LimboError, Result, }; +#[cfg(debug_assertions)] use std::collections::HashSet; use std::{ cell::{Cell, Ref, RefCell}, @@ -338,6 +339,14 @@ enum OverflowState { Done, } +enum CursorContext { + TableRowId(u64), + + /// If we are in an index tree we can then reuse this field to save + /// our cursor information + IndexKeyRowId(ImmutableRecord), +} + pub struct BTreeCursor { /// The multi-version cursor that is used to read and write to the database file. mv_cursor: Option>>, @@ -366,7 +375,8 @@ pub struct BTreeCursor { pub index_key_sort_order: IndexKeySortOrder, /// Maintain count of the number of records in the btree. Used for the `Count` opcode count: usize, - rowid_seen: HashSet, + /// Stores the last record that was seen. + context: Option, } impl BTreeCursor { @@ -393,7 +403,7 @@ impl BTreeCursor { empty_record: Cell::new(true), index_key_sort_order: IndexKeySortOrder::default(), count: 0, - rowid_seen: HashSet::new(), + context: None, } } @@ -759,8 +769,6 @@ impl BTreeCursor { let contents = mem_page.contents.as_ref().unwrap(); let cell_count = contents.cell_count(); - tracing::trace!(cell_idx, cell_count, "get_next_record"); - if cell_count == 0 || cell_idx == cell_count { // do rightmost let has_parent = self.stack.has_parent(); @@ -835,11 +843,6 @@ impl BTreeCursor { )? }; self.stack.advance(); - if self.rowid_seen.contains(_rowid) { - continue; - } else { - self.rowid_seen.insert(*_rowid); - } return Ok(CursorResult::Ok(Some(*_rowid))); } BTreeCell::IndexInteriorCell(IndexInteriorCell { @@ -905,11 +908,6 @@ impl BTreeCursor { Some(RefValue::Integer(rowid)) => *rowid as u64, _ => unreachable!("index cells should have an integer rowid"), }; - if self.rowid_seen.contains(&rowid) { - continue; - } else { - self.rowid_seen.insert(rowid); - } return Ok(CursorResult::Ok(Some(rowid))); } else { continue; @@ -970,11 +968,6 @@ impl BTreeCursor { Some(RefValue::Integer(rowid)) => *rowid as u64, _ => unreachable!("index cells should have an integer rowid"), }; - if self.rowid_seen.contains(&rowid) { - continue; - } else { - self.rowid_seen.insert(rowid); - } return Ok(CursorResult::Ok(Some(rowid))); } else { continue; @@ -3440,7 +3433,6 @@ impl BTreeCursor { } pub fn rewind(&mut self) -> Result> { - self.rowid_seen.clear(); if self.mv_cursor.is_some() { let rowid = return_if_io!(self.get_next_record(None)); self.rowid.replace(rowid); @@ -3464,6 +3456,7 @@ impl BTreeCursor { } pub fn next(&mut self) -> Result> { + let _ = self.restore_context()?; let rowid = return_if_io!(self.get_next_record(None)); self.rowid.replace(rowid); self.empty_record.replace(rowid.is_none()); @@ -3557,6 +3550,7 @@ impl BTreeCursor { /// 8. Finish -> Delete operation is done. Return CursorResult(Ok()) pub fn delete(&mut self) -> Result> { assert!(self.mv_cursor.is_none()); + self.save_context(); if let CursorState::None = &self.state { self.state = CursorState::Delete(DeleteInfo { @@ -4359,6 +4353,44 @@ impl BTreeCursor { } } } + + /// Save cursor context, to be restored later + pub fn save_context(&mut self) { + if let Some(rowid) = self.rowid.get() { + match self.stack.top().get_contents().page_type() { + PageType::TableInterior | PageType::TableLeaf => { + self.context = Some(CursorContext::TableRowId(rowid)); + } + PageType::IndexInterior | PageType::IndexLeaf => { + self.context = Some(CursorContext::IndexKeyRowId( + self.reusable_immutable_record + .borrow() + .as_ref() + .unwrap() + .clone(), + )); + } + } + } + } + + /// If context is defined, restore it and set it None on success + fn restore_context(&mut self) -> Result> { + if self.context.is_none() { + return Ok(CursorResult::Ok(false)); + } + let ctx = self.context.as_ref().unwrap(); + let res = match ctx { + CursorContext::TableRowId(rowid) => self.seek(SeekKey::TableRowId(*rowid), SeekOp::EQ), + CursorContext::IndexKeyRowId(record) => { + // TODO: see how to avoid clone here + let record = record.clone(); + self.seek(SeekKey::IndexKey(&record), SeekOp::EQ) + } + }?; + self.context = None; + Ok(res) + } } #[cfg(debug_assertions)] @@ -4461,11 +4493,6 @@ impl PageStack { /// Cell index of the current page fn current_cell_index(&self) -> i32 { let current = self.current(); - tracing::event!( - tracing::Level::TRACE, - "current: cell_indices={:?}", - self.cell_indices - ); self.cell_indices.borrow()[current] } @@ -4486,11 +4513,6 @@ impl PageStack { self.cell_indices ); self.cell_indices.borrow_mut()[current] += 1; - tracing::event!( - tracing::Level::TRACE, - "after: cell_indices={:?}", - self.cell_indices - ); } fn retreat(&self) { @@ -4501,11 +4523,6 @@ impl PageStack { self.cell_indices ); self.cell_indices.borrow_mut()[current] -= 1; - tracing::event!( - tracing::Level::TRACE, - "after: cell_indices={:?}", - self.cell_indices - ); } /// Move the cursor to the next cell in the current page according to the iteration direction. @@ -4522,18 +4539,7 @@ impl PageStack { fn set_cell_index(&self, idx: i32) { let current = self.current(); - tracing::event!( - tracing::Level::TRACE, - "set_cell_index={} cell_indices={:?}", - idx, - self.cell_indices - ); self.cell_indices.borrow_mut()[current] = idx; - tracing::event!( - tracing::Level::TRACE, - "after: cell_indices={:?}", - self.cell_indices - ); } fn has_parent(&self) -> bool { diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index ec6eb1c7f..1196394e0 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -593,6 +593,7 @@ 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 { let index_cursor = program.alloc_cursor_id( Some(index.table_name.clone()), @@ -603,7 +604,8 @@ fn emit_program_for_update( root_page: RegisterOrLiteral::Literal(index.root_page), name: index.name.clone(), }); - index_cursors.push(index_cursor); + let record_reg = program.alloc_register(); + index_cursors.push((index_cursor, record_reg)); } open_loop( program, @@ -644,7 +646,7 @@ fn emit_update_insns( plan: &UpdatePlan, t_ctx: &TranslateCtx, program: &mut ProgramBuilder, - index_cursors: Vec, + index_cursors: Vec<(usize, usize)>, ) -> crate::Result<()> { let table_ref = &plan.table_references.first().unwrap(); let loop_labels = t_ctx.labels_main_loop.first().unwrap(); @@ -671,46 +673,6 @@ fn emit_update_insns( _ => return Ok(()), }; - // THIS IS SAME CODE AS WE HAVE ON INSERT - // allocate cursor id's for each btree index cursor we'll need to populate the indexes - // (idx name, root_page, idx cursor id) - let idx_cursors = plan - .indexes_to_update - .iter() - .map(|idx| { - let record_reg = program.alloc_register(); - ( - &idx.name, - idx.root_page, - program.alloc_cursor_id( - Some(idx.table_name.clone()), - CursorType::BTreeIndex(idx.clone()), - ), - record_reg, - ) - }) - .collect::>(); - - if !idx_cursors.is_empty() { - let open_indices_label = program.allocate_label(); - - // Open all cursors Once - program.emit_insn(Insn::Once { - target_pc_when_reentered: open_indices_label, - }); - - // Open all the index btrees for writing - for idx_cursor in idx_cursors.iter() { - program.emit_insn(Insn::OpenWrite { - cursor_id: idx_cursor.2, - root_page: idx_cursor.1.into(), - name: idx_cursor.0.clone(), - }); - } - - program.preassign_label_to_next_insn(open_indices_label); - } - for cond in plan .where_clause .iter() @@ -797,86 +759,6 @@ fn emit_update_insns( }); } - for (pos, index) in plan.indexes_to_update.iter().enumerate() { - if index.unique { - let (_, _, idx_cursor_id, record_reg) = - idx_cursors.get(pos).expect("index cursor should exist"); - - let num_cols = index.columns.len(); - // allocate scratch registers for the index columns plus rowid - let idx_start_reg = program.alloc_registers(num_cols + 1); - - let rowid_reg = beg; - let idx_cols_start_reg = beg + 1; - - // copy each index column from the table's column registers into these scratch regs - for (i, col) in index.columns.iter().enumerate() { - // copy from the table's column register over to the index's scratch register - - program.emit_insn(Insn::Copy { - src_reg: idx_cols_start_reg + col.pos_in_table, - dst_reg: idx_start_reg + i, - amount: 0, - }); - } - // last register is the rowid - program.emit_insn(Insn::Copy { - src_reg: rowid_reg, - dst_reg: idx_start_reg + num_cols, - amount: 0, - }); - - program.emit_insn(Insn::MakeRecord { - start_reg: idx_start_reg, - count: num_cols + 1, - dest_reg: *record_reg, - index_name: Some(index.name.clone()), - }); - - let constraint_check = program.allocate_label(); - program.emit_insn(Insn::NoConflict { - cursor_id: *idx_cursor_id, - target_pc: constraint_check, - record_reg: idx_start_reg, - num_regs: num_cols, - }); - - let column_names = index.columns.iter().enumerate().fold( - String::with_capacity(50), - |mut accum, (idx, col)| { - if idx > 0 { - accum.push_str(", "); - } - accum.push_str(&table_ref.table.get_name()); - accum.push('.'); - accum.push_str(&col.name); - - accum - }, - ); - - let idx_rowid_reg = program.alloc_register(); - program.emit_insn(Insn::IdxRowId { - cursor_id: *idx_cursor_id, - dest: idx_rowid_reg, - }); - - program.emit_insn(Insn::Eq { - lhs: rowid_reg, - rhs: idx_rowid_reg, - target_pc: constraint_check, - flags: CmpInsFlags::default(), // TODO: not sure what type of comparison flag is needed - }); - - program.emit_insn(Insn::Halt { - err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index for error code - description: column_names, - }); - - program.preassign_label_to_next_insn(constraint_check); - } - } - for cond in plan .where_clause .iter() @@ -896,47 +778,6 @@ fn emit_update_insns( )?; } - // Update indexes first. Columns that are updated will be translated from an expression and those who aren't modified will be - // read from table. Mutiple value index key could be updated partially. - for (index, index_cursor) in plan.indexes_to_update.iter().zip(index_cursors) { - let index_record_reg_count = index.columns.len() + 1; - let index_record_reg_start = program.alloc_registers(index_record_reg_count); - for (idx, column) in index.columns.iter().enumerate() { - if let Some((_, expr)) = plan.set_clauses.iter().find(|(i, _)| *i == idx) { - translate_expr( - program, - Some(&plan.table_references), - expr, - index_record_reg_start + idx, - &t_ctx.resolver, - )?; - } else { - program.emit_insn(Insn::Column { - cursor_id: cursor_id, - column: column.pos_in_table, - dest: index_record_reg_start + idx, - }); - } - } - program.emit_insn(Insn::RowId { - cursor_id: cursor_id, - dest: index_record_reg_start + index.columns.len(), - }); - let index_record_reg = program.alloc_register(); - program.emit_insn(Insn::MakeRecord { - start_reg: index_record_reg_start, - count: index_record_reg_count, - dest_reg: index_record_reg, - index_name: Some(index.name.clone()), - }); - program.emit_insn(Insn::IdxInsert { - cursor_id: index_cursor, - record_reg: index_record_reg, - unpacked_start: Some(index_record_reg_start), - unpacked_count: Some(index_record_reg_count as u16), - flags: IdxInsertFlags::new(), - }); - } // we scan a column at a time, loading either the column's values, or the new value // from the Set expression, into registers so we can emit a MakeRecord and update the row. let start = if is_virtual { beg + 2 } else { beg + 1 }; @@ -1006,11 +847,8 @@ fn emit_update_insns( } } - for (pos, index) in plan.indexes_to_update.iter().enumerate() { + for (index, (idx_cursor_id, record_reg)) in plan.indexes_to_update.iter().zip(&index_cursors) { if index.unique { - let (_, _, idx_cursor_id, record_reg) = - idx_cursors.get(pos).expect("index cursor should exist"); - let num_cols = index.columns.len(); // allocate scratch registers for the index columns plus rowid let idx_start_reg = program.alloc_registers(num_cols + 1); @@ -1148,7 +986,8 @@ fn emit_update_insns( } // For each index -> insert - for (pos, index) in plan.indexes_to_update.iter().enumerate() { + for (index, (idx_cursor_id, record_reg)) in plan.indexes_to_update.iter().zip(index_cursors) + { let num_regs = index.columns.len() + 1; let start_reg = program.alloc_registers(num_regs); @@ -1170,18 +1009,15 @@ fn emit_update_insns( dest: start_reg + num_regs - 1, }); - let (_, _, idx_cursor_id, record_reg) = - idx_cursors.get(pos).expect("index cursor should exist"); - program.emit_insn(Insn::IdxDelete { start_reg, num_regs, - cursor_id: *idx_cursor_id, + cursor_id: idx_cursor_id, }); program.emit_insn(Insn::IdxInsert { - cursor_id: *idx_cursor_id, - record_reg: *record_reg, + cursor_id: idx_cursor_id, + record_reg: record_reg, unpacked_start: Some(start), unpacked_count: Some((index.columns.len() + 1) as u16), flags: IdxInsertFlags::new(), From b2615d7739f65a0a2764019deb4c145bb72adfe7 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Sun, 11 May 2025 12:57:29 -0300 Subject: [PATCH 16/18] add CursorValidState and only save context in delete when rebalancing --- core/storage/btree.rs | 17 ++++++++++++++--- core/translate/index.rs | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index eb6c996d1..f149fd4c7 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -347,6 +347,12 @@ enum CursorContext { IndexKeyRowId(ImmutableRecord), } +// In the future, we may expand these general validity states +enum CursorValidState { + Valid, + RequireSeek, +} + pub struct BTreeCursor { /// The multi-version cursor that is used to read and write to the database file. mv_cursor: Option>>, @@ -377,6 +383,8 @@ pub struct BTreeCursor { count: usize, /// Stores the last record that was seen. context: Option, + /// Store whether the Cursor is in a valid state + valid_state: CursorValidState, } impl BTreeCursor { @@ -404,6 +412,7 @@ impl BTreeCursor { index_key_sort_order: IndexKeySortOrder::default(), count: 0, context: None, + valid_state: CursorValidState::Valid, } } @@ -3550,7 +3559,6 @@ impl BTreeCursor { /// 8. Finish -> Delete operation is done. Return CursorResult(Ok()) pub fn delete(&mut self) -> Result> { assert!(self.mv_cursor.is_none()); - self.save_context(); if let CursorState::None = &self.state { self.state = CursorState::Delete(DeleteInfo { @@ -3764,13 +3772,14 @@ impl BTreeCursor { write_info.state = WriteState::BalanceStart; delete_info.balance_write_info = Some(write_info); } - delete_info.state = DeleteState::WaitForBalancingToComplete { target_key } } else { self.stack.retreat(); self.state = CursorState::None; return Ok(CursorResult::Ok(())); } + // Only reaches this function call if state = DeleteState::WaitForBalancingToComplete + self.save_context(); } DeleteState::WaitForBalancingToComplete { target_key } => { @@ -4357,6 +4366,7 @@ impl BTreeCursor { /// Save cursor context, to be restored later pub fn save_context(&mut self) { if let Some(rowid) = self.rowid.get() { + self.valid_state = CursorValidState::RequireSeek; match self.stack.top().get_contents().page_type() { PageType::TableInterior | PageType::TableLeaf => { self.context = Some(CursorContext::TableRowId(rowid)); @@ -4376,7 +4386,7 @@ impl BTreeCursor { /// If context is defined, restore it and set it None on success fn restore_context(&mut self) -> Result> { - if self.context.is_none() { + if self.context.is_none() || !matches!(self.valid_state, CursorValidState::RequireSeek) { return Ok(CursorResult::Ok(false)); } let ctx = self.context.as_ref().unwrap(); @@ -4389,6 +4399,7 @@ impl BTreeCursor { } }?; self.context = None; + self.valid_state = CursorValidState::Valid; Ok(res) } } diff --git a/core/translate/index.rs b/core/translate/index.rs index f104120a0..30e96f897 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -378,6 +378,7 @@ pub fn translate_drop_index( program.emit_insn(Insn::OpenWrite { cursor_id: sqlite_schema_cursor_id, root_page: RegisterOrLiteral::Literal(sqlite_table.root_page), + name: sqlite_table.name.clone(), }); let loop_start_label = program.allocate_label(); From 72cc0fcdcb8f03fc418839661d30b4618fedcfc0 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Mon, 12 May 2025 11:29:24 -0300 Subject: [PATCH 17/18] fixes and comments --- core/storage/btree.rs | 18 +++-- core/translate/emitter.rs | 136 +++++++++++++++++++------------------- 2 files changed, 80 insertions(+), 74 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index f149fd4c7..11388f457 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -339,6 +339,8 @@ enum OverflowState { Done, } +/// Holds a Record or RowId, so that these can be transformed into a SeekKey to restore +/// cursor position to its previous location. enum CursorContext { TableRowId(u64), @@ -347,9 +349,11 @@ enum CursorContext { IndexKeyRowId(ImmutableRecord), } -// In the future, we may expand these general validity states +/// In the future, we may expand these general validity states enum CursorValidState { + /// Cursor is pointing a to an existing location/cell in the Btree Valid, + /// Cursor may be pointing to a non-existent location/cell. This can happen after balancing operations RequireSeek, } @@ -381,9 +385,9 @@ pub struct BTreeCursor { pub index_key_sort_order: IndexKeySortOrder, /// Maintain count of the number of records in the btree. Used for the `Count` opcode count: usize, - /// Stores the last record that was seen. + /// Stores the cursor context before rebalancing so that a seek can be done later context: Option, - /// Store whether the Cursor is in a valid state + /// Store whether the Cursor is in a valid state. Meaning if it is pointing to a valid cell index or not valid_state: CursorValidState, } @@ -4385,12 +4389,12 @@ impl BTreeCursor { } /// If context is defined, restore it and set it None on success - fn restore_context(&mut self) -> Result> { + fn restore_context(&mut self) -> Result> { if self.context.is_none() || !matches!(self.valid_state, CursorValidState::RequireSeek) { - return Ok(CursorResult::Ok(false)); + return Ok(CursorResult::Ok(())); } let ctx = self.context.as_ref().unwrap(); - let res = match ctx { + let _ = match ctx { CursorContext::TableRowId(rowid) => self.seek(SeekKey::TableRowId(*rowid), SeekOp::EQ), CursorContext::IndexKeyRowId(record) => { // TODO: see how to avoid clone here @@ -4400,7 +4404,7 @@ impl BTreeCursor { }?; self.context = None; self.valid_state = CursorValidState::Valid; - Ok(res) + Ok(CursorResult::Ok(())) } } diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 1196394e0..7c48f92ad 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -848,80 +848,82 @@ fn emit_update_insns( } for (index, (idx_cursor_id, record_reg)) in plan.indexes_to_update.iter().zip(&index_cursors) { - if index.unique { - let num_cols = index.columns.len(); - // allocate scratch registers for the index columns plus rowid - let idx_start_reg = program.alloc_registers(num_cols + 1); + if !index.unique { + continue; + } - let rowid_reg = beg; - let idx_cols_start_reg = beg + 1; + let num_cols = index.columns.len(); + // allocate scratch registers for the index columns plus rowid + let idx_start_reg = program.alloc_registers(num_cols + 1); - // copy each index column from the table's column registers into these scratch regs - for (i, col) in index.columns.iter().enumerate() { - // copy from the table's column register over to the index's scratch register + let rowid_reg = beg; + let idx_cols_start_reg = beg + 1; + + // copy each index column from the table's column registers into these scratch regs + for (i, col) in index.columns.iter().enumerate() { + // copy from the table's column register over to the index's scratch register - program.emit_insn(Insn::Copy { - src_reg: idx_cols_start_reg + col.pos_in_table, - dst_reg: idx_start_reg + i, - amount: 0, - }); - } - // last register is the rowid program.emit_insn(Insn::Copy { - src_reg: rowid_reg, - dst_reg: idx_start_reg + num_cols, + src_reg: idx_cols_start_reg + col.pos_in_table, + dst_reg: idx_start_reg + i, amount: 0, }); - - program.emit_insn(Insn::MakeRecord { - start_reg: idx_start_reg, - count: num_cols + 1, - dest_reg: *record_reg, - index_name: Some(index.name.clone()), - }); - - let constraint_check = program.allocate_label(); - program.emit_insn(Insn::NoConflict { - cursor_id: *idx_cursor_id, - target_pc: constraint_check, - record_reg: idx_start_reg, - num_regs: num_cols, - }); - - let column_names = index.columns.iter().enumerate().fold( - String::with_capacity(50), - |mut accum, (idx, col)| { - if idx > 0 { - accum.push_str(", "); - } - accum.push_str(&table_ref.table.get_name()); - accum.push('.'); - accum.push_str(&col.name); - - accum - }, - ); - - let idx_rowid_reg = program.alloc_register(); - program.emit_insn(Insn::IdxRowId { - cursor_id: *idx_cursor_id, - dest: idx_rowid_reg, - }); - - program.emit_insn(Insn::Eq { - lhs: rowid_reg, - rhs: idx_rowid_reg, - target_pc: constraint_check, - flags: CmpInsFlags::default(), // TODO: not sure what type of comparison flag is needed - }); - - program.emit_insn(Insn::Halt { - err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index for error code - description: column_names, - }); - - program.preassign_label_to_next_insn(constraint_check); } + // last register is the rowid + program.emit_insn(Insn::Copy { + src_reg: rowid_reg, + dst_reg: idx_start_reg + num_cols, + amount: 0, + }); + + program.emit_insn(Insn::MakeRecord { + start_reg: idx_start_reg, + count: num_cols + 1, + dest_reg: *record_reg, + index_name: Some(index.name.clone()), + }); + + let constraint_check = program.allocate_label(); + program.emit_insn(Insn::NoConflict { + cursor_id: *idx_cursor_id, + target_pc: constraint_check, + record_reg: idx_start_reg, + num_regs: num_cols, + }); + + let column_names = index.columns.iter().enumerate().fold( + String::with_capacity(50), + |mut accum, (idx, col)| { + if idx > 0 { + accum.push_str(", "); + } + accum.push_str(&table_ref.table.get_name()); + accum.push('.'); + accum.push_str(&col.name); + + accum + }, + ); + + let idx_rowid_reg = program.alloc_register(); + program.emit_insn(Insn::IdxRowId { + cursor_id: *idx_cursor_id, + dest: idx_rowid_reg, + }); + + program.emit_insn(Insn::Eq { + lhs: rowid_reg, + rhs: idx_rowid_reg, + target_pc: constraint_check, + flags: CmpInsFlags::default(), // TODO: not sure what type of comparison flag is needed + }); + + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_PRIMARYKEY, // TODO: distinct between primary key and unique index for error code + description: column_names, + }); + + program.preassign_label_to_next_insn(constraint_check); } if let Some(btree_table) = table_ref.btree() { From 9fc9415b20242bbc68fd9d50094e27078cc4b9f9 Mon Sep 17 00:00:00 2001 From: pedrocarlo Date: Tue, 13 May 2025 13:15:01 -0300 Subject: [PATCH 18/18] use Jussi's code to avoid cloning immutable record --- core/storage/btree.rs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/core/storage/btree.rs b/core/storage/btree.rs index 11388f457..7ecaf9463 100644 --- a/core/storage/btree.rs +++ b/core/storage/btree.rs @@ -4393,18 +4393,22 @@ impl BTreeCursor { if self.context.is_none() || !matches!(self.valid_state, CursorValidState::RequireSeek) { return Ok(CursorResult::Ok(())); } - let ctx = self.context.as_ref().unwrap(); - let _ = match ctx { - CursorContext::TableRowId(rowid) => self.seek(SeekKey::TableRowId(*rowid), SeekOp::EQ), - CursorContext::IndexKeyRowId(record) => { - // TODO: see how to avoid clone here - let record = record.clone(); - self.seek(SeekKey::IndexKey(&record), SeekOp::EQ) + let ctx = self.context.take().unwrap(); + let seek_key = match ctx { + CursorContext::TableRowId(rowid) => SeekKey::TableRowId(rowid), + CursorContext::IndexKeyRowId(ref record) => SeekKey::IndexKey(record), + }; + let res = self.seek(seek_key, SeekOp::EQ)?; + match res { + CursorResult::Ok(_) => { + self.valid_state = CursorValidState::Valid; + Ok(CursorResult::Ok(())) } - }?; - self.context = None; - self.valid_state = CursorValidState::Valid; - Ok(CursorResult::Ok(())) + CursorResult::IO => { + self.context = Some(ctx); + Ok(CursorResult::IO) + } + } } }