Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] sql: fix update of a self-referenced table
@ 2020-02-18 17:55 Chris Sosnin
  2020-02-18 19:11 ` Konstantin Osipov
  0 siblings, 1 reply; 2+ messages in thread
From: Chris Sosnin @ 2020-02-18 17:55 UTC (permalink / raw)
  To: korablev, tarantool-patches

An update of a self-referenced table is not handled correctly:
if a new row doesn't reference itself, it is considered to violate
the constraint. This patch makes vdbe to inspect the table on updates
too with a little change: the row we find should be different from the
one we are updating.

Closes #4661
---
I've tried to refactor a patch a little, shortened the comments under
the 66 characters line, rewrote some of them completely. I agree with
Vlad that FK code looks quite complicated (that's why it took me so
long to write the first version of the patch), so if it's needed, I
can try to give a verbal explanation.

branch:https://github.com/tarantool/tarantool/tree/ksosnin/gh-4661-fk-self-ref-table
issue:https://github.com/tarantool/tarantool/issues/4661

 src/box/sql/fk_constraint.c    | 51 ++++++++++++++++++++++++++++------
 test/sql/foreign-keys.result   | 44 +++++++++++++++++++++++++++++
 test/sql/foreign-keys.test.lua | 12 ++++++++
 3 files changed, 99 insertions(+), 8 deletions(-)

diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 482220a95..2e1d8d053 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -225,7 +225,8 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 	 * we are about to increment the constraint-counter (i.e.
 	 * this is an INSERT operation), then check if the row
 	 * being inserted matches itself. If so, do not increment
-	 * the constraint-counter.
+	 * the constraint-counter. Otherwise, we must inspect the
+	 * table.
 	 *
 	 * If any of the parent-key values are NULL, then the row
 	 * cannot match itself. So set JUMPIFNULL to make sure we
@@ -244,14 +245,14 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 		}
 		sqlVdbeGoto(v, ok_label);
 	}
-	/**
+	/*
 	 * Inspect a parent table with OP_Found.
-	 * We mustn't make it for a self-referenced table since
-	 * it's tuple will be modified by the update operation.
-	 * And since the foreign key has already detected a
-	 * conflict, fk counter must be increased.
+	 * We mustn't make it for a self-referenced
+	 * table on deletion during an update operation
+	 * since it's tuple will be modified later.
 	 */
-	if (!(fk_constraint_is_self_referenced(fk_def) && is_update)) {
+	if (!(fk_constraint_is_self_referenced(fk_def) && is_update &&
+	      incr_count == -1)) {
 		int temp_regs = sqlGetTempRange(parse_context, field_count);
 		int rec_reg = sqlGetTempReg(parse_context);
 		vdbe_emit_open_cursor(parse_context, cursor, referenced_idx,
@@ -269,7 +270,41 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 				  (char *) sql_index_type_str(parse_context->db,
 							      idx->def),
 				  P4_DYNAMIC);
-		sqlVdbeAddOp4Int(v, OP_Found, cursor, ok_label, rec_reg, 0);
+
+		/*
+		 * In this particular case, we should check, that
+		 * the row we found and the row being updated
+		 * are different, otherwise the update operation
+		 * will violate the constraint. This check is
+		 * correct, since we've already verified that
+		 * the row doesn't match itself.
+		 */
+		bool check_not_equal = incr_count == 1 && is_update &&
+				       fk_constraint_is_self_referenced(fk_def);
+
+		int jump = sqlVdbeCurrentAddr(v) + 1;
+		sqlVdbeAddOp4Int(v, OP_Found, cursor,
+				 check_not_equal ? jump + 1 : ok_label,
+				 rec_reg, 0);
+		if (check_not_equal) {
+			/*
+			 * We've found nothing, there's no need
+			 * to compare rows.
+			 */
+			sqlVdbeGoto(v, jump + 4);
+			/*
+			 * Otherwise, cursor is pointing to the
+			 * found entry, compare its index to the
+			 * index of the tuple being updated.
+			 */
+			int idx_reg = sqlGetTempReg(parse_context);
+			sqlVdbeAddOp3(v, OP_Column, cursor, referenced_idx,
+				      idx_reg);
+			sqlVdbeAddOp3(v, OP_Eq, reg_data + referenced_idx + 1,
+				      jump + 4, idx_reg);
+			sqlReleaseTempReg(parse_context, idx_reg);
+			sqlVdbeGoto(v, ok_label);
+		}
 		sqlReleaseTempReg(parse_context, rec_reg);
 		sqlReleaseTempRange(parse_context, temp_regs, field_count);
 	}
diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index f1d973443..a9b10ee38 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -429,6 +429,50 @@ box.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
 box.space.T4:drop()
 ---
 ...
+-- gh-4661: UPDATE must work correctly on self-referenced spaces.
+--
+box.execute('CREATE TABLE t5(a INT UNIQUE, b INT REFERENCES t5(a), id INT PRIMARY KEY AUTOINCREMENT)')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t5(a,b) VALUES(1, null)')
+---
+- autoincrement_ids:
+  - 1
+  row_count: 1
+...
+box.execute('INSERT INTO t5(a,b) VALUES(2, null)')
+---
+- autoincrement_ids:
+  - 2
+  row_count: 1
+...
+box.execute('UPDATE t5 SET b = 2 WHERE a = 1')
+---
+- row_count: 1
+...
+box.execute('UPDATE t5 SET b = 1 WHERE a = 2')
+---
+- row_count: 1
+...
+box.execute('INSERT INTO t5(a,b) VALUES(3, 2)')
+---
+- autoincrement_ids:
+  - 3
+  row_count: 1
+...
+box.execute('UPDATE t5 SET a = 4 WHERE a = 3')
+---
+- row_count: 1
+...
+box.execute('UPDATE t5 SET b = 4 WHERE a = 1')
+---
+- row_count: 1
+...
+box.execute('DROP TABLE t5')
+---
+- row_count: 1
+...
 -- Make sure that child space can feature no PK.
 --
 t1 = box.schema.create_space("T1", {format = {'ID'}})
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index d2dd88d28..4fb651787 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -179,6 +179,18 @@ box.space.T1:drop()
 box.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
 box.space.T4:drop()
 
+-- gh-4661: UPDATE must work correctly on self-referenced spaces.
+--
+box.execute('CREATE TABLE t5(a INT UNIQUE, b INT REFERENCES t5(a), id INT PRIMARY KEY AUTOINCREMENT)')
+box.execute('INSERT INTO t5(a,b) VALUES(1, null)')
+box.execute('INSERT INTO t5(a,b) VALUES(2, null)')
+box.execute('UPDATE t5 SET b = 2 WHERE a = 1')
+box.execute('UPDATE t5 SET b = 1 WHERE a = 2')
+box.execute('INSERT INTO t5(a,b) VALUES(3, 2)')
+box.execute('UPDATE t5 SET a = 4 WHERE a = 3')
+box.execute('UPDATE t5 SET b = 4 WHERE a = 1')
+box.execute('DROP TABLE t5')
+
 -- Make sure that child space can feature no PK.
 --
 t1 = box.schema.create_space("T1", {format = {'ID'}})
-- 
2.21.1 (Apple Git-122.3)

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-02-18 19:11 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 17:55 [Tarantool-patches] [PATCH] sql: fix update of a self-referenced table Chris Sosnin
2020-02-18 19:11 ` Konstantin Osipov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox