From f56f37fae5cf83f8169fb0e86f57a4345cf27731 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Wed, 1 Oct 2025 12:59:08 -0400 Subject: [PATCH] Add more tests for self-referencing FKs and remove unneeded FkIfZero checks/labels in emitter --- core/translate/emitter.rs | 56 +++++++++++++++++++++---------- testing/foreign_keys.test | 70 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs index 3a1d1d017..21107843d 100644 --- a/core/translate/emitter.rs +++ b/core/translate/emitter.rs @@ -1039,12 +1039,6 @@ pub fn emit_fk_parent_existence_checks( parent_cursor_id: usize, parent_rowid_reg: usize, ) -> Result<()> { - let after_all = program.allocate_label(); - program.emit_insn(Insn::FkIfZero { - target_pc: after_all, - if_zero: true, - }); - let parent_bt = resolver .schema .get_btree_table(parent_table_name) @@ -1130,6 +1124,25 @@ pub fn emit_fk_parent_existence_checks( extra_amount: 0, }); } + if let Some(count) = NonZeroUsize::new(parent_cols_len) { + // Apply index affinities for composite comparison + let aff: String = idx + .columns + .iter() + .map(|ic| { + let (_, col) = fk_ref + .child_table + .get_column(&ic.name) + .expect("indexed child column not found"); + col.affinity().aff_mask() + }) + .collect(); + program.emit_insn(Insn::Affinity { + start_reg: probe_start, + count, + affinities: aff, + }); + } let ok = program.allocate_label(); program.emit_insn(Insn::NotFound { @@ -1205,7 +1218,7 @@ pub fn emit_fk_parent_existence_checks( lhs: tmp, rhs: parent_key_start + i, target_pc: cont_i, - flags: CmpInsFlags::default(), + flags: CmpInsFlags::default().jump_if_null(), collation: program.curr_collation(), }); // Not equal -> skip this child row @@ -1242,7 +1255,6 @@ pub fn emit_fk_parent_existence_checks( program.emit_insn(Insn::Close { cursor_id: ccur }); } } - program.resolve_label(after_all, program.offset()); Ok(()) } @@ -2339,12 +2351,6 @@ pub fn emit_fk_child_existence_checks( rowid_reg: usize, updated_cols: &HashSet, ) -> Result<()> { - let after_all = program.allocate_label(); - program.emit_insn(Insn::FkIfZero { - target_pc: after_all, - if_zero: true, - }); - for fk_ref in resolver.schema.resolved_fks_for_child(table_name) { // Skip when the child key is untouched (including rowid-alias special case) if !fk_ref.child_key_changed(updated_cols, table) { @@ -2387,11 +2393,17 @@ pub fn emit_fk_child_existence_checks( } else { start_reg + i_child }; - + let tmp = program.alloc_register(); + program.emit_insn(Insn::Copy { + src_reg: val_reg, + dst_reg: tmp, + extra_amount: 0, + }); + program.emit_insn(Insn::MustBeInt { reg: tmp }); let violation = program.allocate_label(); program.emit_insn(Insn::NotExists { cursor: pcur, - rowid_reg: val_reg, + rowid_reg: tmp, target_pc: violation, }); program.emit_insn(Insn::Close { cursor_id: pcur }); @@ -2440,6 +2452,16 @@ pub fn emit_fk_child_existence_checks( }); } + let aff: String = parent_idx + .columns + .iter() + .map(|ic| table.columns[ic.pos_in_table].affinity().aff_mask()) + .collect(); + program.emit_insn(Insn::Affinity { + start_reg: probe_start, + count: NonZeroUsize::new(n).unwrap(), + affinities: aff, + }); let found = program.allocate_label(); program.emit_insn(Insn::Found { cursor_id: icur, @@ -2471,8 +2493,6 @@ pub fn emit_fk_child_existence_checks( program.preassign_label_to_next_insn(fk_ok); } - - program.resolve_label(after_all, program.offset()); Ok(()) } diff --git a/testing/foreign_keys.test b/testing/foreign_keys.test index a88ca55fe..78e8498f2 100644 --- a/testing/foreign_keys.test +++ b/testing/foreign_keys.test @@ -299,7 +299,7 @@ do_execsql_test_in_memory_any_error fk-rowid-mustbeint-coercion-fail { CREATE TABLE p(id INTEGER PRIMARY KEY); CREATE TABLE c(cid INTEGER PRIMARY KEY, pid REFERENCES p(id)); INSERT INTO p(id) VALUES(10); - INSERT INTO c VALUES(2, 'abc'); -- MustBeInt fails to match any parent row + INSERT INTO c VALUES(2, 'abc'); -- MustBeInt fails to match any parent row } # Parent match via UNIQUE index (non-rowid), success path @@ -331,3 +331,71 @@ do_execsql_test_on_specific_db {:memory:} fk-child-null-shortcircuit { INSERT INTO c VALUES (1, NULL); -- NULL child is allowed SELECT id, pid FROM c; } {1|} + +do_execsql_test_on_specific_db {:memory:} fk-self-unique-ok { + PRAGMA foreign_keys=ON; + CREATE TABLE t( + u TEXT, + v TEXT, + cu TEXT, + cv TEXT, + UNIQUE(u,v), + FOREIGN KEY(cu,cv) REFERENCES t(u,v) + ); + -- Single row insert where child points to its own (u,v): allowed + INSERT INTO t(u,v,cu,cv) VALUES('A','B','A','B'); + SELECT u, v, cu, cv FROM t; +} {A|B|A|B} + +do_execsql_test_in_memory_any_error fk-self-unique-mismatch { + PRAGMA foreign_keys=ON; + CREATE TABLE t( + u TEXT, + v TEXT, + cu TEXT, + cv TEXT, + UNIQUE(u,v), + FOREIGN KEY(cu,cv) REFERENCES t(u,v) + ); + -- Child points to a different (u,v) that doesn't exist: must fail + INSERT INTO t(u,v,cu,cv) VALUES('A','B','A','X'); +} + +do_execsql_test_on_specific_db {:memory:} fk-self-unique-reference-existing-ok { + PRAGMA foreign_keys=ON; + CREATE TABLE t( + u TEXT, + v TEXT, + cu TEXT, + cv TEXT, + UNIQUE(u,v), + FOREIGN KEY(cu,cv) REFERENCES t(u,v) + ); + -- Insert a parent row first + INSERT INTO t(u,v,cu,cv) VALUES('P','Q',NULL,NULL); + -- Now insert a row whose FK references the existing ('P','Q'): OK + INSERT INTO t(u,v,cu,cv) VALUES('X','Y','P','Q'); + SELECT u, v, cu, cv FROM t ORDER BY u, v, cu, cv; +} {P|Q|| X|Y|P|Q} + +do_execsql_test_on_specific_db {:memory:} fk-self-unique-multirow-no-fastpath { + PRAGMA foreign_keys=ON; + CREATE TABLE t( + u TEXT, + v TEXT, + cu TEXT, + cv TEXT, + UNIQUE(u,v), + FOREIGN KEY(cu,cv) REFERENCES t(u,v) + ); + INSERT INTO t(u,v,cu,cv) VALUES + ('C','D','C','D'), + ('E','F','E','F'); +} {} + +do_execsql_test_in_memory_any_error fk-self-multirow-one-bad { + PRAGMA foreign_keys=ON; + CREATE TABLE t(id INTEGER PRIMARY KEY, rid INTEGER, + FOREIGN KEY(rid) REFERENCES t(id)); + INSERT INTO t(id,rid) VALUES (1,1),(3,99); -- 99 has no parent -> error +}