From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 87974469719 for ; Tue, 18 Feb 2020 20:55:14 +0300 (MSK) From: Chris Sosnin Date: Tue, 18 Feb 2020 20:55:11 +0300 Message-Id: <20200218175511.55344-1-k.sosnin@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH] sql: fix update of a self-referenced table List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: korablev@tarantool.org, tarantool-patches@dev.tarantool.org 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)