From 5bff10c56efc2d80a0808ec505694ae7286be558 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sat, 15 Nov 2025 20:51:23 -0500 Subject: [PATCH 1/9] Implement INSERT OR REPLACE translation/emission --- core/translate/insert.rs | 248 ++++++++++++++++++++++++++++++++++++--- testing/insert.test | 95 ++++++++++++++- 2 files changed, 326 insertions(+), 17 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index db1d829ee..6304af2d8 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -9,15 +9,16 @@ use crate::error::{ }; use crate::schema::{self, BTreeTable, ColDef, Index, ResolvedFkRef, Table}; use crate::translate::emitter::{ - emit_cdc_insns, emit_cdc_patch_record, prepare_cdc_if_necessary, OperationMode, + emit_cdc_full_record, emit_cdc_insns, emit_cdc_patch_record, emit_fk_child_decrement_on_delete, + prepare_cdc_if_necessary, OperationMode, }; use crate::translate::expr::{ bind_and_rewrite_expr, emit_returning_results, process_returning_clause, walk_expr_mut, BindingBehavior, WalkControl, }; use crate::translate::fkeys::{ - build_index_affinity_string, emit_fk_violation, emit_guarded_fk_decrement, index_probe, - open_read_index, open_read_table, + build_index_affinity_string, emit_fk_delete_parent_existence_checks, emit_fk_violation, + emit_guarded_fk_decrement, index_probe, open_read_index, open_read_table, }; use crate::translate::plan::{ ColumnUsedMask, JoinedTable, Operation, ResultSetColumn, TableReferences, @@ -126,7 +127,6 @@ pub struct InsertEmitCtx<'a> { pub key_generation_label: BranchOffset, /// Jump here when the insert value SELECT source has been fully exhausted pub select_exhausted_label: Option, - /// CDC table info pub cdc_table: Option<(usize, Arc)>, /// Autoincrement sequence table info @@ -368,9 +368,10 @@ pub fn translate_insert( &insertion, &upsert_actions, &constraints, + connection, )?; - emit_notnulls(&mut program, &ctx, &insertion); + emit_notnulls(&mut program, &ctx, &insertion, resolver)?; // Create and insert the record let affinity_str = insertion @@ -401,11 +402,15 @@ pub fn translate_insert( )?; } + let mut insert_flags = InsertFlags::new(); + if matches!(ctx.on_conflict, ResolveType::Replace) { + insert_flags = insert_flags.require_seek(); + } program.emit_insn(Insn::Insert { cursor: ctx.cursor_id, key_reg: insertion.key_register(), record_reg: insertion.record_register(), - flag: InsertFlags::new(), + flag: insert_flags, table_name: table_name.to_string(), }); @@ -876,7 +881,13 @@ fn init_autoincrement( Ok(()) } -fn emit_notnulls(program: &mut ProgramBuilder, ctx: &InsertEmitCtx, insertion: &Insertion) { +fn emit_notnulls( + program: &mut ProgramBuilder, + ctx: &InsertEmitCtx, + insertion: &Insertion, + resolver: &Resolver, +) -> Result<()> { + let on_replace = matches!(ctx.on_conflict, ResolveType::Replace); for column_mapping in insertion .col_mappings .iter() @@ -886,6 +897,32 @@ fn emit_notnulls(program: &mut ProgramBuilder, ctx: &InsertEmitCtx, insertion: & if column_mapping.column.is_rowid_alias() { continue; } + if on_replace { + if let Some(default_expr) = column_mapping.column.default.as_ref() { + // OR REPLACE + NOT NULL + DEFAULT: + // if reg IS NOT NULL -> skip + // if reg IS NULL -> evaluate DEFAULT into this register + let ok = program.allocate_label(); + + program.emit_insn(Insn::NotNull { + reg: column_mapping.register, + target_pc: ok, + }); + + // At this point, value is NULL: overwrite with default expression + translate_expr( + program, + None, + default_expr, + column_mapping.register, + resolver, + )?; + + program.preassign_label_to_next_insn(ok); + continue; + } + // OR REPLACE but no DEFAULT: fall through to ABORT behavior below + } program.emit_insn(Insn::HaltIfNull { target_reg: column_mapping.register, err_code: SQLITE_CONSTRAINT_NOTNULL, @@ -913,6 +950,7 @@ fn emit_notnulls(program: &mut ProgramBuilder, ctx: &InsertEmitCtx, insertion: & }, }); } + Ok(()) } struct BoundInsertResult { @@ -1005,6 +1043,7 @@ fn bind_insert( ResolveType::Abort => { // This is the default conflict resolution strategy for INSERT in SQLite. } + ResolveType::Replace => {} _ => { crate::bail_parse_error!( "INSERT OR {} is only supported with UPSERT", @@ -1683,7 +1722,9 @@ fn emit_preflight_constraint_checks( insertion: &Insertion, upsert_actions: &[(ResolvedUpsertTarget, BranchOffset, Box)], constraints: &ConstraintsToCheck, + connection: &Arc, ) -> Result<()> { + let on_replace = matches!(ctx.on_conflict, ResolveType::Replace) && upsert_actions.is_empty(); for (constraint, position) in &constraints.constraints_to_check { match constraint { ResolvedUpsertTarget::PrimaryKey => { @@ -1698,6 +1739,24 @@ fn emit_preflight_constraint_checks( // Conflict on rowid: attempt to route through UPSERT if it targets the PK, otherwise raise constraint. // emit Halt for every case *except* when upsert handles the conflict 'emit_halt: { + if on_replace { + // copy the conflicting rowid into the key register and delete the existing row inline + program.emit_insn(Insn::Copy { + src_reg: insertion.key_register(), + dst_reg: ctx.conflict_rowid_reg, + extra_amount: 0, + }); + emit_replace_delete_conflicting_row( + program, + resolver, + connection, + ctx, + )?; + program.emit_insn(Insn::Goto { + target_pc: make_record_label, + }); + break 'emit_halt; + } if let Some(position) = position.or(constraints.upsert_catch_all_position) { // PK conflict: the conflicting rowid is exactly the attempted key program.emit_insn(Insn::Copy { @@ -1804,7 +1863,6 @@ fn emit_preflight_constraint_checks( record_reg: idx_start_reg, num_regs: num_cols, }); - // Conflict detected, figure out if this UPSERT handles the conflict if let Some(position) = position.or(constraints.upsert_catch_all_position) { match &upsert_actions[position].2.do_clause { @@ -1847,14 +1905,28 @@ fn emit_preflight_constraint_checks( record_reg: idx_start_reg, num_regs: num_cols, }); - // Unique violation without ON CONFLICT clause -> error - program.emit_insn(Insn::Halt { - err_code: SQLITE_CONSTRAINT_UNIQUE, - description: format_unique_violation_desc( - ctx.table.name.as_str(), - index, - ), - }); + if on_replace { + program.emit_insn(Insn::IdxRowId { + cursor_id: idx_cursor_id, + dest: ctx.conflict_rowid_reg, + }); + emit_replace_delete_conflicting_row( + program, + resolver, + connection, + ctx, + )?; + program.emit_insn(Insn::Goto { target_pc: ok }); + } else { + // Unique violation without ON CONFLICT clause -> error + program.emit_insn(Insn::Halt { + err_code: SQLITE_CONSTRAINT_UNIQUE, + description: format_unique_violation_desc( + ctx.table.name.as_str(), + index, + ), + }); + } program.preassign_label_to_next_insn(ok); // In the non-UPSERT case, we insert the index @@ -2258,6 +2330,150 @@ fn emit_update_sqlite_sequence( Ok(()) } +fn emit_replace_delete_conflicting_row( + program: &mut ProgramBuilder, + resolver: &Resolver, + connection: &Arc, + ctx: &InsertEmitCtx, +) -> Result<()> { + program.emit_insn(Insn::SeekRowid { + cursor_id: ctx.cursor_id, + src_reg: ctx.conflict_rowid_reg, + target_pc: ctx.halt_label, + }); + + emit_delete_single_row( + connection, + program, + ctx, + resolver, + ctx.conflict_rowid_reg, + true, + ) +} + +fn emit_delete_single_row( + connection: &Arc, + program: &mut ProgramBuilder, + ctx: &InsertEmitCtx, + resolver: &Resolver, + rowid_reg: usize, + is_part_of_update: bool, +) -> Result<()> { + let table = &ctx.table; + let table_name = table.name.as_str(); + let main_cursor_id = ctx.cursor_id; + + if connection.foreign_keys_enabled() { + if resolver.schema.any_resolved_fks_referencing(table_name) { + emit_fk_delete_parent_existence_checks( + program, + resolver, + table_name, + main_cursor_id, + rowid_reg, + )?; + } + if resolver.schema.has_child_fks(table_name) { + emit_fk_child_decrement_on_delete( + program, + resolver, + table, + table_name, + main_cursor_id, + rowid_reg, + )?; + } + } + for (name, _, index_cursor_id) in ctx.idx_cursors.iter() { + let index = resolver + .schema + .get_index(table_name, name) + .expect("index to exist"); + let skip_delete_label = if index.where_clause.is_some() { + let where_copy = index + .bind_where_expr(None, connection) + .expect("where clause to exist"); + let skip_label = program.allocate_label(); + let reg = program.alloc_register(); + translate_expr_no_constant_opt( + program, + None, + &where_copy, + reg, + resolver, + NoConstantOptReason::RegisterReuse, + )?; + program.emit_insn(Insn::IfNot { + reg, + jump_if_null: true, + target_pc: skip_label, + }); + Some(skip_label) + } else { + None + }; + + let num_regs = index.columns.len() + 1; + let start_reg = program.alloc_registers(num_regs); + + for (reg_offset, column_index) in index.columns.iter().enumerate() { + program.emit_column_or_rowid( + main_cursor_id, + column_index.pos_in_table, + start_reg + reg_offset, + ); + } + program.emit_insn(Insn::Copy { + src_reg: rowid_reg, + dst_reg: start_reg + num_regs - 1, + extra_amount: 0, + }); + program.emit_insn(Insn::IdxDelete { + start_reg, + num_regs, + cursor_id: *index_cursor_id, + raise_error_if_no_matching_entry: index.where_clause.is_none(), + }); + + if let Some(label) = skip_delete_label { + program.resolve_label(label, program.offset()); + } + } + + // CDC BEFORE, using rowid_reg + if let Some(cdc_cursor_id) = ctx.cdc_table.as_ref().map(|(id, _tbl)| *id) { + let cdc_has_before = program.capture_data_changes_mode().has_before(); + let before_record_reg = if cdc_has_before { + Some(emit_cdc_full_record( + program, + &table.columns, + main_cursor_id, + rowid_reg, + )) + } else { + None + }; + emit_cdc_insns( + program, + resolver, + OperationMode::DELETE, + cdc_cursor_id, + rowid_reg, + before_record_reg, + None, + None, + table_name, + )?; + } + program.emit_insn(Insn::Delete { + cursor_id: main_cursor_id, + table_name: table_name.to_string(), + is_part_of_update, + }); + Ok(()) +} + /// Child-side FK checks for INSERT of a single row: /// For each outgoing FK on `child_tbl`, if the NEW tuple's FK columns are all non-NULL, /// verify that the referenced parent key exists. diff --git a/testing/insert.test b/testing/insert.test index e7e5e786a..1268a145e 100755 --- a/testing/insert.test +++ b/testing/insert.test @@ -871,4 +871,97 @@ do_execsql_test_on_specific_db {:memory:} insert-999999999 { create table t(a); insert into t(a, a) values (2, 3); select * from t; -} {2} \ No newline at end of file +} {2} + +do_execsql_test_on_specific_db {:memory:} insert-on-conflict-do-nothing-single-row { + CREATE TABLE t(a unique, b text); + INSERT INTO t VALUES (2, 'foo'),(3, 'bar'); + INSERT OR IGNORE INTO t values (2, 'baz'); + SELECT * FROM t; +} {2|foo +3|bar} + +do_execsql_test_on_specific_db {:memory:} insert-on-conflict-replace-single-row { + CREATE TABLE t(a unique, b text); + INSERT INTO t VALUES (2, 'foo'),(3, 'bar'); + INSERT OR REPLACE INTO t values (2, 'baz'); + SELECT * FROM t ORDER BY a; +} {2|baz +3|bar} + +do_execsql_test_on_specific_db {:memory:} insert-on-conflict-do-nothing-multiple-rows { + CREATE TABLE t(a unique); + INSERT INTO t VALUES (2),(3); + INSERT OR IGNORE INTO t values (1),(2),(3); + SELECT * FROM t; +} {1 +2 +3} + +do_execsql_test_on_specific_db {:memory:} onconflict-ignore-existing { + CREATE TABLE t(a INTEGER UNIQUE, b TEXT); + INSERT INTO t VALUES (1,'x'),(2,'y'); + INSERT OR IGNORE INTO t VALUES (2,'yy'),(3,'z'),(2,'zzz'); + SELECT a, b FROM t ORDER BY a; +} {1|x +2|y +3|z} + +do_execsql_test_on_specific_db {:memory:} onconflict-ignore-selfdup-values { + CREATE TABLE t(a INTEGER UNIQUE, b TEXT); + INSERT OR IGNORE INTO t VALUES (1,'one'),(1,'two'),(1,'three'); + SELECT a, b FROM t ORDER BY a; +} {1|one} + +do_execsql_test_on_specific_db {:memory:} onconflict-ignore-selfdup-values-targeted { + CREATE TABLE t(a INTEGER PRIMARY KEY, b INTEGER UNIQUE); + INSERT INTO t(a,b) VALUES (100,1); + INSERT INTO t(a,b) VALUES (200,1) ON CONFLICT(b) DO NOTHING; + SELECT a,b FROM t ORDER BY a; +} {100|1} + +do_execsql_test_on_specific_db {:memory:} onconflict-ignore-selfdup-multirow-targeted { + CREATE TABLE t(a INTEGER PRIMARY KEY, b INTEGER UNIQUE); + INSERT INTO t(a,b) VALUES (1,10),(2,20); + INSERT INTO t(a,b) VALUES (3,30),(4,30) ON CONFLICT(b) DO NOTHING; + SELECT a,b FROM t ORDER BY a; +} {1|10 +2|20 +3|30} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-existing { + CREATE TABLE t(a INTEGER UNIQUE, b TEXT); + INSERT INTO t VALUES (1,'x'),(2,'y'); + INSERT OR REPLACE INTO t VALUES (2,'yy'),(3,'z'); + SELECT a,b FROM t ORDER BY a; +} {1|x +2|yy +3|z} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-selfdup-values-lastwins { + CREATE TABLE t(a INTEGER UNIQUE, b TEXT); + INSERT OR REPLACE INTO t VALUES (5,'one'),(5,'two'),(5,'three'); + SELECT a,b FROM t; +} {5|three} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-existing-then-selfdup-lastwins { + CREATE TABLE t(a INTEGER UNIQUE, b TEXT); + INSERT INTO t VALUES (5,'orig'); + INSERT OR REPLACE INTO t VALUES (5,'first'),(5,'second'); + SELECT a,b FROM t; +} {5|second} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-pk-rowid-alias { + CREATE TABLE t(a INTEGER PRIMARY KEY, b TEXT); + INSERT OR REPLACE INTO t(a,b) VALUES (1,'foo'),(1,'bar'); + SELECT a,b FROM t; +} {1|bar} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-from-select-selfdup { + CREATE TABLE src(x); + INSERT INTO src VALUES (1),(1),(2); + CREATE TABLE t(a INTEGER UNIQUE); + INSERT OR REPLACE INTO t SELECT x FROM src; + SELECT a FROM t ORDER BY a; +} {1 +2} From 634af4d6f6b3fedce2d08c2e27493a9f28c44c55 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 16 Nov 2025 15:17:08 -0500 Subject: [PATCH 2/9] Handle NOT NULL behavior for INSERT OR REPLACE --- core/translate/insert.rs | 61 ++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 6304af2d8..f28ae8354 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -371,7 +371,7 @@ pub fn translate_insert( connection, )?; - emit_notnulls(&mut program, &ctx, &insertion, resolver)?; + let notnull_resume_label = emit_notnulls(&mut program, &ctx, &insertion, resolver)?; // Create and insert the record let affinity_str = insertion @@ -379,6 +379,10 @@ pub fn translate_insert( .iter() .map(|col_mapping| col_mapping.column.affinity().aff_mask()) .collect::(); + + if let Some(lbl) = notnull_resume_label { + program.preassign_label_to_next_insn(lbl); + } program.emit_insn(Insn::MakeRecord { start_reg: insertion.first_col_register(), count: insertion.col_mappings.len(), @@ -886,8 +890,9 @@ fn emit_notnulls( ctx: &InsertEmitCtx, insertion: &Insertion, resolver: &Resolver, -) -> Result<()> { +) -> Result> { let on_replace = matches!(ctx.on_conflict, ResolveType::Replace); + let mut pending_resume_label = None; for column_mapping in insertion .col_mappings .iter() @@ -897,31 +902,47 @@ fn emit_notnulls( if column_mapping.column.is_rowid_alias() { continue; } + + // If a NOT NULL constraint violation occurs, the REPLACE conflict resolution replaces the NULL value with the default value for that column, + // or if the column has no default value, then the ABORT algorithm is used if on_replace { if let Some(default_expr) = column_mapping.column.default.as_ref() { - // OR REPLACE + NOT NULL + DEFAULT: - // if reg IS NOT NULL -> skip - // if reg IS NULL -> evaluate DEFAULT into this register - let ok = program.allocate_label(); + let default_label = { + if let Some(lbl) = pending_resume_label { + lbl + } else { + program.allocate_label() + } + }; + let resume_label = program.allocate_label(); - program.emit_insn(Insn::NotNull { + program.emit_insn(Insn::IsNull { reg: column_mapping.register, - target_pc: ok, + target_pc: default_label, }); - // At this point, value is NULL: overwrite with default expression - translate_expr( + program.emit_insn(Insn::Goto { + target_pc: resume_label, + }); + + program.resolve_label(default_label, program.offset()); + + // Evaluate default expression into the column register. + translate_expr_no_constant_opt( program, None, default_expr, column_mapping.register, resolver, + NoConstantOptReason::RegisterReuse, )?; - program.preassign_label_to_next_insn(ok); - continue; + program.emit_insn(Insn::Goto { + target_pc: resume_label, + }); + pending_resume_label = Some(resume_label); } - // OR REPLACE but no DEFAULT: fall through to ABORT behavior below + // OR REPLACE but no DEFAULT, fall through to ABORT behavior } program.emit_insn(Insn::HaltIfNull { target_reg: column_mapping.register, @@ -950,7 +971,7 @@ fn emit_notnulls( }, }); } - Ok(()) + Ok(pending_resume_label) } struct BoundInsertResult { @@ -1746,12 +1767,7 @@ fn emit_preflight_constraint_checks( dst_reg: ctx.conflict_rowid_reg, extra_amount: 0, }); - emit_replace_delete_conflicting_row( - program, - resolver, - connection, - ctx, - )?; + emit_replace_delete_conflicting_row(program, resolver, connection, ctx)?; program.emit_insn(Insn::Goto { target_pc: make_record_label, }); @@ -1911,10 +1927,7 @@ fn emit_preflight_constraint_checks( dest: ctx.conflict_rowid_reg, }); emit_replace_delete_conflicting_row( - program, - resolver, - connection, - ctx, + program, resolver, connection, ctx, )?; program.emit_insn(Insn::Goto { target_pc: ok }); } else { From b83921d83870bf0f4a3a005daec6323c94d4edf2 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 16 Nov 2025 15:17:29 -0500 Subject: [PATCH 3/9] Add TCL tests for insert or replace handling --- testing/insert.test | 88 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-) diff --git a/testing/insert.test b/testing/insert.test index 1268a145e..d4be00388 100755 --- a/testing/insert.test +++ b/testing/insert.test @@ -893,7 +893,7 @@ do_execsql_test_on_specific_db {:memory:} insert-on-conflict-do-nothing-multiple CREATE TABLE t(a unique); INSERT INTO t VALUES (2),(3); INSERT OR IGNORE INTO t values (1),(2),(3); - SELECT * FROM t; + SELECT * FROM t order by a; } {1 2 3} @@ -965,3 +965,89 @@ do_execsql_test_on_specific_db {:memory:} onconflict-replace-from-select-selfdup SELECT a FROM t ORDER BY a; } {1 2} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-notnull-default-existing-null { + CREATE TABLE t( + a INTEGER PRIMARY KEY, + b TEXT NOT NULL DEFAULT 'd' + ); + INSERT INTO t VALUES (1,'x'); + INSERT OR REPLACE INTO t(a,b) VALUES (1,NULL); + SELECT a,b FROM t; +} {1|d} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-notnull-default-existing-omit { + CREATE TABLE t( + a INTEGER PRIMARY KEY, + b TEXT NOT NULL DEFAULT 'd' + ); + INSERT INTO t VALUES (1,'x'); + INSERT OR REPLACE INTO t(a) VALUES (1); + SELECT a,b FROM t; +} {1|d} + + +# need to assert row survival after failed REPLACE due to NOT NULL constraint +# so we create table t here +do_execsql_test 1.0 { + CREATE TABLE t(a INTEGER PRIMARY KEY, b TEXT NOT NULL); + INSERT INTO t VALUES (1,'x'); +} {} + +do_execsql_test_any_error 1.1 { + INSERT OR REPLACE INTO t(a,b) VALUES (1,NULL); +} + +do_execsql_test 1.2 { + SELECT a,b FROM t; +} {1|x} + +# drop the table created above as not to alter the testing db +do_execsql_test drop-t { + DROP TABLE t; +} {} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-composite-unique-existing { + CREATE TABLE t( + a INTEGER, + b INTEGER, + c TEXT, + UNIQUE(a,b) + ); + INSERT INTO t VALUES (1,1,'x'),(1,2,'y'); + INSERT OR REPLACE INTO t VALUES (1,2,'yy'); + SELECT a,b,c FROM t ORDER BY a,b; +} {1|1|x +1|2|yy} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-composite-unique-selfdup-lastwins { + CREATE TABLE t( + a INTEGER, + b INTEGER, + c TEXT, + UNIQUE(a,b) + ); + INSERT OR REPLACE INTO t VALUES + (1,1,'one'), + (1,1,'two'), + (1,1,'three'); + SELECT a,b,c FROM t; +} {1|1|three} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-unique-index-existing { + CREATE TABLE t(a INTEGER, b TEXT); + CREATE UNIQUE INDEX t_a_u ON t(a); + INSERT INTO t VALUES (1,'x'),(2,'y'); + INSERT OR REPLACE INTO t VALUES (2,'yy'),(3,'z'); + SELECT a,b FROM t ORDER BY a; +} {1|x +2|yy +3|z} + +do_execsql_test_on_specific_db {:memory:} onconflict-replace-unique-index-selfdup-lastwins { + CREATE TABLE t(a INTEGER, b TEXT); + CREATE UNIQUE INDEX t_a_u ON t(a); + INSERT OR REPLACE INTO t VALUES (5,'one'),(5,'two'),(5,'three'); + SELECT a,b FROM t; +} {5|three} + From 0bc56d3f287bfcfaadd3af821d81b0c6b2a0c225 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 16 Nov 2025 15:20:02 -0500 Subject: [PATCH 4/9] Sprinkle some OR REPLACE into INSERT statements in fuzzing --- tests/fuzz/mod.rs | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/tests/fuzz/mod.rs b/tests/fuzz/mod.rs index 212f53db5..d116c054c 100644 --- a/tests/fuzz/mod.rs +++ b/tests/fuzz/mod.rs @@ -1100,7 +1100,14 @@ mod fuzz_tests { } let a = rng.random_range(-5..=25); let b = rng.random_range(-5..=25); - format!("INSERT INTO p VALUES({id}, {a}, {b})") + format!( + "INSERT {} INTO p VALUES({id}, {a}, {b})", + if rng.random_bool(0.4) { + "OR REPLACE " + } else { + "" + } + ) } // Parent UPDATE 1 => { @@ -1133,7 +1140,14 @@ mod fuzz_tests { rng.random_range(1..=260) as i64 }; let y = rng.random_range(-10..=10); - format!("INSERT INTO c VALUES({id}, {x}, {y})") + format!( + "INSERT {} INTO c VALUES({id}, {x}, {y})", + if rng.random_bool(0.4) { + "OR REPLACE " + } else { + "" + } + ) } // Child UPDATE 4 => { @@ -1940,7 +1954,12 @@ mod fuzz_tests { .collect::>() .join(", "); let insert = format!( - "INSERT INTO t ({}) VALUES {}", + "INSERT {} INTO t ({}) VALUES {}", + if rng.random_bool(0.4) { + "OR IGNORE" + } else { + "" + }, col_names, insert_values.join(", ") ); @@ -2259,7 +2278,14 @@ mod fuzz_tests { } } format!( - "INSERT INTO t({}) VALUES({})", + "INSERT {} INTO t({}) VALUES({})", + if rng.random_bool(0.3) { + "OR REPLACE" + } else if rng.random_bool(0.3) { + "OR IGNORE" + } else { + "" + }, cols_list.join(","), vals_list.join(",") ) From f8e78b73a8292b56451df6808a3dce824312f404 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 16 Nov 2025 16:11:59 -0500 Subject: [PATCH 5/9] Fix handling of partial indexes when deleting rows in ON REPLACE for insert --- core/translate/insert.rs | 46 ++++++++++++++++++++++++---------------- core/translate/upsert.rs | 3 +-- 2 files changed, 29 insertions(+), 20 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index f28ae8354..5ed47649f 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -29,7 +29,7 @@ use crate::translate::upsert::{ }; use crate::util::normalize_ident; use crate::vdbe::affinity::Affinity; -use crate::vdbe::builder::ProgramBuilderOpts; +use crate::vdbe::builder::{CursorKey, ProgramBuilderOpts}; use crate::vdbe::insn::{CmpInsFlags, IdxInsertFlags, InsertFlags, RegisterOrLiteral}; use crate::vdbe::BranchOffset; use crate::{ @@ -94,6 +94,7 @@ pub struct TempTableCtx { pub struct InsertEmitCtx<'a> { /// Parent table being inserted into pub table: &'a Arc, + pub table_references: &'a mut TableReferences, /// Index cursors we need to populate for this table /// (idx name, root_page, idx cursor id) @@ -134,6 +135,7 @@ pub struct InsertEmitCtx<'a> { } impl<'a> InsertEmitCtx<'a> { + #[allow(clippy::too_many_arguments)] fn new( program: &mut ProgramBuilder, resolver: &Resolver, @@ -142,6 +144,7 @@ impl<'a> InsertEmitCtx<'a> { cdc_table: Option<(usize, Arc)>, num_values: usize, temp_table_ctx: Option, + table_references: &'a mut TableReferences, ) -> Result { // allocate cursor id's for each btree index cursor we'll need to populate the indexes let indices = resolver.schema.get_indices(table.name.as_str()); @@ -161,6 +164,7 @@ impl<'a> InsertEmitCtx<'a> { let key_generation_label = program.allocate_label(); Ok(Self { table, + table_references, idx_cursors, temp_table_ctx, on_conflict: on_conflict.unwrap_or(ResolveType::Abort), @@ -276,6 +280,7 @@ pub fn translate_insert( cdc_table, values.len(), None, + &mut table_references, )?; program = init_source_emission( @@ -363,7 +368,7 @@ pub fn translate_insert( // invalid index entries before we hit a conflict down the line. emit_preflight_constraint_checks( &mut program, - &ctx, + &mut ctx, resolver, &insertion, &upsert_actions, @@ -485,7 +490,7 @@ pub fn translate_insert( if !result_columns.is_empty() { emit_returning_results( &mut program, - &table_references, + ctx.table_references, &result_columns, insertion.first_col_register(), insertion.key_register(), @@ -505,7 +510,6 @@ pub fn translate_insert( &table, &mut result_columns, connection, - &table_references, )?; } @@ -774,7 +778,6 @@ fn resolve_upserts( table: &Table, result_columns: &mut [ResultSetColumn], connection: &Arc, - table_references: &TableReferences, ) -> Result<()> { for (_, label, upsert) in upsert_actions { program.preassign_label_to_next_insn(*label); @@ -797,7 +800,6 @@ fn resolve_upserts( resolver, result_columns, connection, - table_references, )?; } else { // UpsertDo::Nothing case @@ -1061,10 +1063,10 @@ fn bind_insert( next: None, })); } - ResolveType::Abort => { - // This is the default conflict resolution strategy for INSERT in SQLite. + ResolveType::Abort | ResolveType::Replace => { + // Abort is the default conflict resolution strategy for INSERT in SQLite, + // and we implement Replace. } - ResolveType::Replace => {} _ => { crate::bail_parse_error!( "INSERT OR {} is only supported with UPSERT", @@ -1160,7 +1162,10 @@ fn init_source_emission<'a>( { ( values.len(), - program.alloc_cursor_id(CursorType::BTreeTable(ctx.table.clone())), + program.alloc_cursor_id_keyed( + CursorKey::table(ctx.table_references.joined_tables()[0].internal_id), + CursorType::BTreeTable(ctx.table.clone()), + ), ) } else { // Multiple rows - use coroutine for value population @@ -1192,8 +1197,10 @@ fn init_source_emission<'a>( program.emit_insn(Insn::EndCoroutine { yield_reg }); program.preassign_label_to_next_insn(jump_on_definition_label); - - let cursor_id = program.alloc_cursor_id(CursorType::BTreeTable(ctx.table.clone())); + let cursor_id = program.alloc_cursor_id_keyed( + CursorKey::table(ctx.table_references.joined_tables()[0].internal_id), + CursorType::BTreeTable(ctx.table.clone()), + ); // From SQLite /* Set useTempTable to TRUE if the result of the SELECT statement @@ -1320,7 +1327,10 @@ fn init_source_emission<'a>( })); ( num_values, - program.alloc_cursor_id(CursorType::BTreeTable(ctx.table.clone())), + program.alloc_cursor_id_keyed( + CursorKey::table(ctx.table_references.joined_tables()[0].internal_id), + CursorType::BTreeTable(ctx.table.clone()), + ), ) } }; @@ -1738,7 +1748,7 @@ fn translate_column( // otherwise, raise SQLITE_CONSTRAINT_UNIQUE fn emit_preflight_constraint_checks( program: &mut ProgramBuilder, - ctx: &InsertEmitCtx, + ctx: &mut InsertEmitCtx, resolver: &Resolver, insertion: &Insertion, upsert_actions: &[(ResolvedUpsertTarget, BranchOffset, Box)], @@ -2347,7 +2357,7 @@ fn emit_replace_delete_conflicting_row( program: &mut ProgramBuilder, resolver: &Resolver, connection: &Arc, - ctx: &InsertEmitCtx, + ctx: &mut InsertEmitCtx, ) -> Result<()> { program.emit_insn(Insn::SeekRowid { cursor_id: ctx.cursor_id, @@ -2368,7 +2378,7 @@ fn emit_replace_delete_conflicting_row( fn emit_delete_single_row( connection: &Arc, program: &mut ProgramBuilder, - ctx: &InsertEmitCtx, + ctx: &mut InsertEmitCtx, resolver: &Resolver, rowid_reg: usize, is_part_of_update: bool, @@ -2405,13 +2415,13 @@ fn emit_delete_single_row( .expect("index to exist"); let skip_delete_label = if index.where_clause.is_some() { let where_copy = index - .bind_where_expr(None, connection) + .bind_where_expr(Some(ctx.table_references), connection) .expect("where clause to exist"); let skip_label = program.allocate_label(); let reg = program.alloc_register(); translate_expr_no_constant_opt( program, - None, + Some(ctx.table_references), &where_copy, reg, resolver, diff --git a/core/translate/upsert.rs b/core/translate/upsert.rs index 69ced5b81..44f293a68 100644 --- a/core/translate/upsert.rs +++ b/core/translate/upsert.rs @@ -344,7 +344,6 @@ pub fn emit_upsert( resolver: &mut Resolver, returning: &mut [ResultSetColumn], connection: &Arc, - table_references: &TableReferences, ) -> crate::Result<()> { // Seek & snapshot CURRENT program.emit_insn(Insn::SeekRowid { @@ -828,7 +827,7 @@ pub fn emit_upsert( if !returning.is_empty() { emit_returning_results( program, - table_references, + ctx.table_references, returning, new_start, new_rowid_reg.unwrap_or(ctx.conflict_rowid_reg), From 0ce5f810085d76234ce43384d29c9ee04847319e Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Sun, 16 Nov 2025 16:23:22 -0500 Subject: [PATCH 6/9] Cleanup translate/insert fix clippy warnings --- core/translate/insert.rs | 30 ++++++------------------------ core/translate/upsert.rs | 1 - 2 files changed, 6 insertions(+), 25 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 5ed47649f..05e1a88d8 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -2365,24 +2365,6 @@ fn emit_replace_delete_conflicting_row( target_pc: ctx.halt_label, }); - emit_delete_single_row( - connection, - program, - ctx, - resolver, - ctx.conflict_rowid_reg, - true, - ) -} - -fn emit_delete_single_row( - connection: &Arc, - program: &mut ProgramBuilder, - ctx: &mut InsertEmitCtx, - resolver: &Resolver, - rowid_reg: usize, - is_part_of_update: bool, -) -> Result<()> { let table = &ctx.table; let table_name = table.name.as_str(); let main_cursor_id = ctx.cursor_id; @@ -2394,7 +2376,7 @@ fn emit_delete_single_row( resolver, table_name, main_cursor_id, - rowid_reg, + ctx.conflict_rowid_reg, )?; } if resolver.schema.has_child_fks(table_name) { @@ -2404,7 +2386,7 @@ fn emit_delete_single_row( table, table_name, main_cursor_id, - rowid_reg, + ctx.conflict_rowid_reg, )?; } } @@ -2448,7 +2430,7 @@ fn emit_delete_single_row( ); } program.emit_insn(Insn::Copy { - src_reg: rowid_reg, + src_reg: ctx.conflict_rowid_reg, dst_reg: start_reg + num_regs - 1, extra_amount: 0, }); @@ -2472,7 +2454,7 @@ fn emit_delete_single_row( program, &table.columns, main_cursor_id, - rowid_reg, + ctx.conflict_rowid_reg, )) } else { None @@ -2482,7 +2464,7 @@ fn emit_delete_single_row( resolver, OperationMode::DELETE, cdc_cursor_id, - rowid_reg, + ctx.conflict_rowid_reg, before_record_reg, None, None, @@ -2492,7 +2474,7 @@ fn emit_delete_single_row( program.emit_insn(Insn::Delete { cursor_id: main_cursor_id, table_name: table_name.to_string(), - is_part_of_update, + is_part_of_update: true, }); Ok(()) } diff --git a/core/translate/upsert.rs b/core/translate/upsert.rs index 44f293a68..bbc4a17dc 100644 --- a/core/translate/upsert.rs +++ b/core/translate/upsert.rs @@ -10,7 +10,6 @@ use crate::translate::emitter::UpdateRowSource; use crate::translate::expr::{walk_expr, WalkControl}; use crate::translate::fkeys::{emit_fk_child_update_counters, emit_parent_key_change_checks}; use crate::translate::insert::{format_unique_violation_desc, InsertEmitCtx}; -use crate::translate::plan::TableReferences; use crate::translate::planner::ROWID_STRS; use crate::vdbe::insn::CmpInsFlags; use crate::Connection; From 8cd33f3ec9507142ab8eed6ac5739fb7c3ac29b2 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Mon, 17 Nov 2025 08:41:22 -0500 Subject: [PATCH 7/9] Add comment for or replace behavior require seek in translate/insert --- core/translate/insert.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 05e1a88d8..b1897b17f 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -412,6 +412,9 @@ pub fn translate_insert( } let mut insert_flags = InsertFlags::new(); + + // For the case of OR REPLACE, we need to force a seek on the insert, as we may have + // already deleted the conflicting row and the cursor is not guaranteed to be positioned. if matches!(ctx.on_conflict, ResolveType::Replace) { insert_flags = insert_flags.require_seek(); } From c3185d0b8c6f35465f840ab251312ee89020eaa7 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Mon, 17 Nov 2025 12:19:33 -0500 Subject: [PATCH 8/9] Properly handle foreign keys for INSERT OR REPLACE --- core/translate/emitter.rs | 4 +-- core/translate/fkeys.rs | 12 ++++---- core/translate/insert.rs | 63 +++++++++++++++++++-------------------- 3 files changed, 39 insertions(+), 40 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index cd962562d..edae8ecbc 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -609,7 +609,7 @@ pub fn emit_fk_child_decrement_on_delete( // Parent MISSING, decrement is guarded by FkIfZero to avoid underflow program.preassign_label_to_next_insn(missing); program.emit_insn(Insn::Close { cursor_id: pcur }); - emit_guarded_fk_decrement(program, done); + emit_guarded_fk_decrement(program, done, true); program.preassign_label_to_next_insn(done); } else { // Probe parent unique index @@ -657,7 +657,7 @@ pub fn emit_fk_child_decrement_on_delete( num_regs: n, }); program.emit_insn(Insn::Close { cursor_id: icur }); - emit_guarded_fk_decrement(program, ok); + emit_guarded_fk_decrement(program, ok, true); program.preassign_label_to_next_insn(ok); program.emit_insn(Insn::Close { cursor_id: icur }); } diff --git a/core/translate/fkeys.rs b/core/translate/fkeys.rs index ec006b26f..d482b2882 100644 --- a/core/translate/fkeys.rs +++ b/core/translate/fkeys.rs @@ -14,14 +14,14 @@ use crate::{ use std::{collections::HashSet, num::NonZeroUsize, sync::Arc}; #[inline] -pub fn emit_guarded_fk_decrement(program: &mut ProgramBuilder, label: BranchOffset) { +pub fn emit_guarded_fk_decrement(program: &mut ProgramBuilder, label: BranchOffset, deferred: bool) { program.emit_insn(Insn::FkIfZero { - deferred: true, + deferred, target_pc: label, }); program.emit_insn(Insn::FkCounter { increment_value: -1, - deferred: true, + deferred, }); } @@ -508,7 +508,7 @@ fn emit_fk_parent_key_probe( (true, ParentProbePass::New) => { // Guard to avoid underflow if OLD pass didn't increment. let skip = p.allocate_label(); - emit_guarded_fk_decrement(p, skip); + emit_guarded_fk_decrement(p, skip, fk_ref.fk.deferred); p.preassign_label_to_next_insn(skip); } // Immediate FK on NEW pass: nothing to cancel; do nothing. @@ -676,7 +676,7 @@ pub fn emit_fk_child_update_counters( program.preassign_label_to_next_insn(miss); program.emit_insn(Insn::Close { cursor_id: pcur }); let skip = program.allocate_label(); - emit_guarded_fk_decrement(program, skip); + emit_guarded_fk_decrement(program, skip, fk_ref.fk.deferred); program.preassign_label_to_next_insn(skip); program.preassign_label_to_next_insn(join); @@ -703,7 +703,7 @@ pub fn emit_fk_child_update_counters( |_p| Ok(()), |p| { let skip = p.allocate_label(); - emit_guarded_fk_decrement(p, skip); + emit_guarded_fk_decrement(p, skip, fk_ref.fk.deferred); p.preassign_label_to_next_insn(skip); Ok(()) }, diff --git a/core/translate/insert.rs b/core/translate/insert.rs index b1897b17f..ed63b80b6 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -9,8 +9,8 @@ use crate::error::{ }; use crate::schema::{self, BTreeTable, ColDef, Index, ResolvedFkRef, Table}; use crate::translate::emitter::{ - emit_cdc_full_record, emit_cdc_insns, emit_cdc_patch_record, emit_fk_child_decrement_on_delete, - prepare_cdc_if_necessary, OperationMode, + emit_cdc_full_record, emit_cdc_insns, emit_cdc_patch_record, prepare_cdc_if_necessary, + OperationMode, }; use crate::translate::expr::{ bind_and_rewrite_expr, emit_returning_results, process_returning_clause, walk_expr_mut, @@ -412,10 +412,11 @@ pub fn translate_insert( } let mut insert_flags = InsertFlags::new(); + let on_replace = matches!(ctx.on_conflict, ResolveType::Replace); // For the case of OR REPLACE, we need to force a seek on the insert, as we may have // already deleted the conflicting row and the cursor is not guaranteed to be positioned. - if matches!(ctx.on_conflict, ResolveType::Replace) { + if on_replace { insert_flags = insert_flags.require_seek(); } program.emit_insn(Insn::Insert { @@ -428,7 +429,15 @@ pub fn translate_insert( if has_fks { // After the row is actually present, repair deferred counters for children referencing this NEW parent key. - emit_parent_side_fk_decrement_on_insert(&mut program, resolver, &btree_table, &insertion)?; + // For REPLACE: delete increments counters above; the insert path should try to repay + // them, even for immediate/self-ref FKs. + emit_parent_side_fk_decrement_on_insert( + &mut program, + resolver, + &btree_table, + &insertion, + on_replace, + )?; } if let Some(AutoincMeta { @@ -1071,10 +1080,7 @@ fn bind_insert( // and we implement Replace. } _ => { - crate::bail_parse_error!( - "INSERT OR {} is only supported with UPSERT", - on_conflict.to_string() - ); + crate::bail_parse_error!("INSERT OR {} is not yet supported", on_conflict.to_string()); } } while let Some(mut upsert_opt) = upsert.take() { @@ -2368,31 +2374,23 @@ fn emit_replace_delete_conflicting_row( target_pc: ctx.halt_label, }); + // OR REPLACE + foreign keys: + // SQLite does not halt on the delete side, it increments FK counters for any referencing + // children and lets the subsequent insert repair them (or fail if it doesn't). + if connection.foreign_keys_enabled() { + emit_fk_delete_parent_existence_checks( + program, + resolver, + ctx.table.name.as_str(), + ctx.cursor_id, + ctx.conflict_rowid_reg, + )?; + } + let table = &ctx.table; let table_name = table.name.as_str(); let main_cursor_id = ctx.cursor_id; - if connection.foreign_keys_enabled() { - if resolver.schema.any_resolved_fks_referencing(table_name) { - emit_fk_delete_parent_existence_checks( - program, - resolver, - table_name, - main_cursor_id, - ctx.conflict_rowid_reg, - )?; - } - if resolver.schema.has_child_fks(table_name) { - emit_fk_child_decrement_on_delete( - program, - resolver, - table, - table_name, - main_cursor_id, - ctx.conflict_rowid_reg, - )?; - } - } for (name, _, index_cursor_id) in ctx.idx_cursors.iter() { let index = resolver .schema @@ -2744,6 +2742,7 @@ pub fn emit_parent_side_fk_decrement_on_insert( resolver: &Resolver, parent_table: &BTreeTable, insertion: &Insertion, + force_immediate: bool, ) -> crate::Result<()> { for pref in resolver .schema @@ -2754,7 +2753,7 @@ pub fn emit_parent_side_fk_decrement_on_insert( .name .eq_ignore_ascii_case(&parent_table.name); // Skip only when it cannot repair anything: non-deferred and not self-referencing - if !pref.fk.deferred && !is_self_ref { + if !force_immediate && !pref.fk.deferred && !is_self_ref { continue; } let (new_pk_start, n_cols) = @@ -2806,7 +2805,7 @@ pub fn emit_parent_side_fk_decrement_on_insert( // Found: guarded counter decrement program.resolve_label(found, program.offset()); program.emit_insn(Insn::Close { cursor_id: icur }); - emit_guarded_fk_decrement(program, skip); + emit_guarded_fk_decrement(program, skip, pref.fk.deferred); program.resolve_label(skip, program.offset()); } else { // fallback scan :( @@ -2851,7 +2850,7 @@ pub fn emit_parent_side_fk_decrement_on_insert( program.resolve_label(cont, program.offset()); } // Matched one child row: guarded decrement of counter - emit_guarded_fk_decrement(program, next_row); + emit_guarded_fk_decrement(program, next_row, pref.fk.deferred); program.resolve_label(next_row, program.offset()); program.emit_insn(Insn::Next { cursor_id: ccur, From 56f35ad4cd083bb75ddcf8aed6f406093044ae73 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Mon, 17 Nov 2025 12:22:55 -0500 Subject: [PATCH 9/9] cargo fmt --- core/translate/fkeys.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/translate/fkeys.rs b/core/translate/fkeys.rs index d482b2882..e08ab7f43 100644 --- a/core/translate/fkeys.rs +++ b/core/translate/fkeys.rs @@ -14,7 +14,11 @@ use crate::{ use std::{collections::HashSet, num::NonZeroUsize, sync::Arc}; #[inline] -pub fn emit_guarded_fk_decrement(program: &mut ProgramBuilder, label: BranchOffset, deferred: bool) { +pub fn emit_guarded_fk_decrement( + program: &mut ProgramBuilder, + label: BranchOffset, + deferred: bool, +) { program.emit_insn(Insn::FkIfZero { deferred, target_pc: label,