diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 1a30f3913..51b480daf 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -1073,10 +1073,16 @@ fn emit_update_insns( } for (index, (idx_cursor_id, record_reg)) in plan.indexes_to_update.iter().zip(&index_cursors) { + // We need to know whether or not the OLD values satisfied the predicate on the + // partial index, so we can know whether or not to delete the old index entry, + // as well as whether or not the NEW values satisfy the predicate, to determine whether + // or not to insert a new index entry for a partial index let (old_satisfies_where, new_satisfies_where) = if let Some(where_clause) = &index.where_clause { let mut where_copy = where_clause.as_ref().clone(); let mut param_state = ParamState::disallow(); + // This means that we need to bind the column references to a copy of the index Expr, + // so we can emit Insn::Column instructions and refer to the old values. bind_and_rewrite_expr( &mut where_copy, Some(&mut plan.table_references), @@ -1094,6 +1100,9 @@ fn emit_update_insns( )?; let mut new_where = where_clause.as_ref().clone(); + // Now we need to rewrite the Expr::Id and Expr::Qualified/Expr::RowID (from a copy of the original, un-bound `where` expr), + // to refer to the new values, which are already loaded into registers starting at + // `start`. rewrite_where_for_update_registers( &mut new_where, unsafe { &*table_ref }.columns(), @@ -1110,6 +1119,9 @@ fn emit_update_insns( &t_ctx.resolver, )?; + // now we have two registers that tell us whether or not the old and new values satisfy + // the partial index predicate, and we can use those to decide whether or not to + // delete/insert a new index entry for this partial index. (Some(old_satisfied_reg), Some(new_satisfied_reg)) } else { (None, None) @@ -1121,6 +1133,7 @@ fn emit_update_insns( // Handle deletion for partial indexes if let Some(old_satisfied) = old_satisfies_where { skip_delete_label = Some(program.allocate_label()); + // If the old values don't satisfy the WHERE clause, skip the delete program.emit_insn(Insn::IfNot { reg: old_satisfied, target_pc: skip_delete_label.unwrap(), @@ -1146,7 +1159,7 @@ fn emit_update_insns( start_reg: delete_start_reg, num_regs, cursor_id: *idx_cursor_id, - raise_error_if_no_matching_entry: old_satisfies_where.is_none(), + raise_error_if_no_matching_entry: true, }); // Resolve delete skip label if it exists @@ -1157,6 +1170,7 @@ fn emit_update_insns( // Check if we should insert into partial index if let Some(new_satisfied) = new_satisfies_where { skip_insert_label = Some(program.allocate_label()); + // If the new values don't satisfy the WHERE clause, skip the idx insert program.emit_insn(Insn::IfNot { reg: new_satisfied, target_pc: skip_insert_label.unwrap(), @@ -1200,7 +1214,7 @@ fn emit_update_insns( }); // Handle unique constraint - if !index.unique { + if index.unique { let constraint_check = program.allocate_label(); // check if the record already exists in the index for unique indexes and abort if so program.emit_insn(Insn::NoConflict { @@ -1765,6 +1779,10 @@ fn init_limit( } } +/// We have `Expr`s which have *not* had column references bound to them, +/// so they are in the state of Expr::Id/Expr::Qualified, etc, and instead of binding Expr::Column +/// we need to bind Expr::Register, as we have already loaded the *new* column values from the +/// UPDATE statement into registers starting at `columns_start_reg`, which we want to reference. fn rewrite_where_for_update_registers( expr: &mut Expr, columns: &[Column], diff --git a/core/translate/index.rs b/core/translate/index.rs index b25a08882..03e4ac41c 100644 --- a/core/translate/index.rs +++ b/core/translate/index.rs @@ -4,12 +4,15 @@ use crate::schema::Table; use crate::translate::emitter::{ emit_cdc_full_record, emit_cdc_insns, prepare_cdc_if_necessary, OperationMode, Resolver, }; -use crate::translate::expr::{bind_and_rewrite_expr, translate_expr, ParamState}; +use crate::translate::expr::{ + bind_and_rewrite_expr, translate_condition_expr, translate_expr, ConditionMetadata, ParamState, +}; use crate::translate::plan::{ ColumnUsedMask, IterationDirection, JoinedTable, Operation, Scan, TableReferences, }; use crate::vdbe::builder::CursorKey; use crate::vdbe::insn::{CmpInsFlags, Cookie}; +use crate::vdbe::BranchOffset; use crate::SymbolTable; use crate::{ schema::{BTreeTable, Column, Index, IndexColumn, PseudoCursorType, Schema}, @@ -208,20 +211,18 @@ pub fn translate_create_index( // Then insert the record into the sorter let mut skip_row_label = None; if let Some(where_clause) = where_clause { - let reg = program.alloc_register(); let label = program.allocate_label(); - let pr = translate_expr( + translate_condition_expr( &mut program, - Some(&table_references), + &table_references, &where_clause, - reg, + ConditionMetadata { + jump_if_condition_is_true: false, + jump_target_when_false: label, + jump_target_when_true: BranchOffset::Placeholder, + }, &resolver, )?; - program.emit_insn(Insn::IfNot { - reg: pr, - target_pc: label, - jump_if_null: true, - }); skip_row_label = Some(label); } diff --git a/core/translate/update.rs b/core/translate/update.rs index 8f565b630..388e9e3df 100644 --- a/core/translate/update.rs +++ b/core/translate/update.rs @@ -395,13 +395,14 @@ pub fn prepare_update_plan( let mut param = ParamState::disallow(); let mut tr = TableReferences::new(table_references.joined_tables().to_vec(), vec![]); - let _ = bind_and_rewrite_expr( + bind_and_rewrite_expr( &mut where_copy, Some(&mut tr), None, connection, &mut param, - ); + ) + .ok()?; let cols_used = collect_cols_used_in_expr(&where_copy); // if any of the columns used in the partial index WHERE clause is being // updated, we need to update this index @@ -447,6 +448,8 @@ fn build_scan_op(table: &Table, iter_dir: IterationDirection) -> Operation { } } +/// Returns a set of column indices used in the expression. +/// *Must* be used on an Expr already processed by `bind_and_rewrite_expr` fn collect_cols_used_in_expr(expr: &Expr) -> HashSet { let mut acc = HashSet::new(); let _ = walk_expr(expr, &mut |expr| match expr {