[Tarantool-patches] [PATCH] sql: fix update of a self-referenced table

[Tarantool-patches] [PATCH] sql: fix update of a self-referenced table

[Chris Sosnin]

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)

Re: [Tarantool-patches] [PATCH] sql: fix update of a self-referenced table

[Konstantin Osipov]

* Chris Sosnin <k.sosnin@tarantool.org> [20/02/18 20:56]:
> 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.

How is tarantool different from upstream that it needs this fix? 
Why is the bug not present in the upstream (I assume it isn't,
it's quite obvious)?

Did you file a bug in the upstream if it's present in it?


-- 
Konstantin Osipov, Moscow, Russia