From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id A3A0320FE7 for ; Sun, 22 Apr 2018 03:37:25 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Cr7bLRIjhgRK for ; Sun, 22 Apr 2018 03:37:25 -0400 (EDT) Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 3617D20FDB for ; Sun, 22 Apr 2018 03:37:24 -0400 (EDT) From: Bulat Niatshin Subject: [tarantool-patches] [PATCH] sql: allow ON CONFLICT REPLACE only for PK index Date: Sun, 22 Apr 2018 10:37:19 +0300 Message-Id: <20180422073719.13245-1-niatshin@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: korablev@tarantool.org, Bulat Niatshin The problem is that constraints which require VDBE bytecode will be executed before making insertion into Tarantool. For three major constraints - UNIQUE, FOREIGN-KEY, CHECK the execution order doesn't matter if error action is not REPLACE, because one constraint violation doesn't affect others. But violation of constraint with ON CONFLICT REPLACE leads to a removal of whole tuple. When only PRIMARY KEY has REPLACE as error action, it is not a problem and such behavior is absolutely similar to Tarantool. But when we are dealing with secondary index /NOT NULL constraint with ON CONFLICT REPLACE, conflict in secondary index leads to a whole tuple removal, and as a result when there is also a conflict in primary key index, the insertion will lead to success instead of raising error. In this patch possibility to create constraint non PRIMARY KEY constraint with ON CONFLICT REPLACE was banned, that kind of behavior will lead to a proper error message. Closes #2963 --- Branch: https://github.com/tarantool/tarantool/tree/bn/gh-2963-proper-replace Issue: https://github.com/tarantool/tarantool/issues/2963 src/box/sql/build.c | 50 ++++++++ test/sql-tap/conflict3.test.lua | 273 ++++------------------------------------ test/sql-tap/default.test.lua | 2 +- test/sql/on-conflict.result | 7 +- test/sql/on-conflict.test.lua | 4 +- 5 files changed, 77 insertions(+), 259 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a2b712a4b..f27916789 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -166,6 +166,47 @@ sqlite3NestedParse(Parse * pParse, const char *zFormat, ...) pParse->nested--; } +/** + * This is a function which should be called during execution + * of sqlite3EndTable. It ensures that only PRIMARY KEY + * constraint may have ON CONFLICT REPLACE clause. + * @param table Space which should be checked. + * @retval bool - false, if only primary key constraint has + * ON CONFLICT REPLACE clause or if there are no + * indexes with REPLACE as error action. In + * other cases it returns true. + */ +static bool +check_on_conflict_replace_entries(Table * table) +{ + Index * idx; + int i; + + /* Check all NOT NULL constraints */ + for (i = 0; i < table->nCol; i++) { + u8 on_error = table->aCol[i].notNull; + if (on_error == ON_CONFLICT_ACTION_REPLACE && + table->aCol[i].is_primkey == false) { + return true; + } + } + + /* Check all UNIQUE constraints */ + for (idx = table->pIndex; idx; idx = idx->pNext) { + if (idx->onError == ON_CONFLICT_ACTION_REPLACE && + !IsPrimaryKeyIndex(idx)) { + return true; + } + } + + /* + * CHECK constraints are not allowed to have REPLACE as + * error action and therefore can be skipped. + */ + + return false; +} + /* * Locate the in-memory structure that describes a particular database * table given the name of that table. Return NULL if not found. @@ -1874,6 +1915,15 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } } + if (check_on_conflict_replace_entries(p)) { + sqlite3ErrorMsg(pParse, + "only PRIMARY KEY constraint can " + "have ON CONFLICT REPLACE clause " + "- %s", + p->zName); + return; + } + #ifndef SQLITE_OMIT_CHECK /* Resolve names in all CHECK constraint expressions. */ diff --git a/test/sql-tap/conflict3.test.lua b/test/sql-tap/conflict3.test.lua index c6c6d019a..345537eac 100755 --- a/test/sql-tap/conflict3.test.lua +++ b/test/sql-tap/conflict3.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(44) +test:plan(29) --!./tcltestrunner.lua -- 2013-11-05 @@ -353,281 +353,50 @@ test:do_execsql_test( -- }) --- Change which column is the PRIMARY KEY --- -test:do_execsql_test( +test:do_catchsql_test( "conflict-7.1", [[ - DROP TABLE t1; - CREATE TABLE t1( - a UNIQUE ON CONFLICT REPLACE, - b INTEGER PRIMARY KEY ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; + CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE, + b UNIQUE ON CONFLICT REPLACE); ]], { - -- - 1, 2, 3, 2, 3, 4 - -- + 1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" }) --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( +test:do_catchsql_test( "conflict-7.2", [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; + CREATE TABLE t3(a PRIMARY KEY, + b UNIQUE ON CONFLICT REPLACE); ]], { - -- - 1, 2, 3, 2, 3, 4 - -- + 1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" }) --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- test:do_catchsql_test( "conflict-7.3", [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- - 1, "UNIQUE constraint failed: T1.C" - -- - }) - -test:do_execsql_test( - "conflict-7.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- - }) - --- Change which column is the PRIMARY KEY --- -test:do_execsql_test( - "conflict-8.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a UNIQUE ON CONFLICT REPLACE, - b INT PRIMARY KEY ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-8.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-8.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- - 1, "UNIQUE constraint failed: T1.C" - -- - }) - -test:do_execsql_test( - "conflict-8.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- - }) - --- Change which column is the PRIMARY KEY --- -test:do_execsql_test( - "conflict-9.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a UNIQUE ON CONFLICT REPLACE, - b INT PRIMARY KEY ON CONFLICT IGNORE, - c UNIQUE ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-9.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- -test:do_catchsql_test( - "conflict-9.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- - 1, "UNIQUE constraint failed: T1.C" - -- - }) - -test:do_execsql_test( - "conflict-9.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- - }) - --- Change which column is the PRIMARY KEY --- -test:do_execsql_test( - "conflict-10.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a UNIQUE ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE, - c INTEGER PRIMARY KEY ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; + CREATE TABLE t3(a PRIMARY KEY, + b UNIQUE ON CONFLICT REPLACE, + c UNIQUE ON CONFLICT REPLACE); ]], { - -- - 1, 2, 3, 2, 3, 4 - -- + 1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" }) --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-10.2", - [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- test:do_catchsql_test( - "conflict-10.3", - [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); - ]], { - -- - 1, "UNIQUE constraint failed: T1.C" - -- - }) - -test:do_execsql_test( - "conflict-10.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- - }) - --- Change which column is the PRIMARY KEY --- -test:do_execsql_test( - "conflict-11.1", - [[ - DROP TABLE t1; - CREATE TABLE t1( - a UNIQUE ON CONFLICT REPLACE, - b UNIQUE ON CONFLICT IGNORE, - c PRIMARY KEY ON CONFLICT FAIL - ); - INSERT INTO t1(a,b,c) VALUES(1,2,3), (2,3,4); - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4 - -- - }) - --- Insert a row that conflicts on column B. The insert should be ignored. --- -test:do_execsql_test( - "conflict-11.2", + "conflict-7.4", [[ - INSERT INTO t1(a,b,c) VALUES(3,2,5); - SELECT a,b,c FROM t1 ORDER BY a; + CREATE TABLE t3(a PRIMARY KEY, + b NOT NULL ON CONFLICT REPLACE DEFAULT 1488); ]], { - -- - 1, 2, 3, 2, 3, 4 - -- + 1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" }) --- Insert two rows where the second conflicts on C. The first row show go --- and and then there should be a constraint error. --- test:do_catchsql_test( - "conflict-11.3", + "conflict-7.5", [[ - INSERT INTO t1(a,b,c) VALUES(4,5,6), (5,6,4); + CREATE TABLE t3(a PRIMARY KEY ON CONFLICT REPLACE, + b NOT NULL ON CONFLICT REPLACE DEFAULT 1488); ]], { - -- - 1, "UNIQUE constraint failed: T1.C" - -- + 1, "only PRIMARY KEY constraint can have ON CONFLICT REPLACE clause - T3" }) -test:do_execsql_test( - "conflict-11.4", - [[ - SELECT a,b,c FROM t1 ORDER BY a; - ]], { - -- - 1, 2, 3, 2, 3, 4, 4, 5, 6 - -- - }) - - - test:finish_test() diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua index 31a91bd14..9d59767ef 100755 --- a/test/sql-tap/default.test.lua +++ b/test/sql-tap/default.test.lua @@ -103,7 +103,7 @@ test:do_execsql_test( CREATE TABLE t3( a INTEGER PRIMARY KEY AUTOINCREMENT, b INT DEFAULT 12345 UNIQUE NOT NULL CHECK( b>=0 AND b<99999 ), - c VARCHAR(123,456) DEFAULT 'hello' NOT NULL ON CONFLICT REPLACE, + c VARCHAR(123,456) DEFAULT 'hello' NOT NULL, d REAL, e FLOATING POINT(5,10) DEFAULT 4.36, f NATIONAL CHARACTER(15), --COLLATE RTRIM, diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result index 5e892eada..9e15ec4d8 100644 --- a/test/sql/on-conflict.result +++ b/test/sql/on-conflict.result @@ -11,7 +11,7 @@ box.sql.execute("CREATE TABLE q (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CON box.sql.execute("CREATE TABLE p (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT IGNORE)") --- ... -box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY, v INTEGER NOT NULL ON CONFLICT REPLACE DEFAULT 1337)") +box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY ON CONFLICT REPLACE, v INTEGER)") --- ... -- Insert values and select them @@ -41,14 +41,13 @@ box.sql.execute("SELECT * FROM p") - [2, 2] - [4, 5] ... -box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (3, 1)") +box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (1, 3)") --- ... box.sql.execute("SELECT * FROM e") --- -- - [1, 1] +- - [1, 3] - [2, 2] - - [3, 1] ... box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)") --- diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua index 1ff199d32..a6aa3d624 100644 --- a/test/sql/on-conflict.test.lua +++ b/test/sql/on-conflict.test.lua @@ -4,7 +4,7 @@ test_run = require('test_run').new() box.sql.execute("CREATE TABLE t (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT ABORT)") box.sql.execute("CREATE TABLE q (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT FAIL)") box.sql.execute("CREATE TABLE p (id INTEGER PRIMARY KEY, v INTEGER UNIQUE ON CONFLICT IGNORE)") -box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY, v INTEGER NOT NULL ON CONFLICT REPLACE DEFAULT 1337)") +box.sql.execute("CREATE TABLE e (id INTEGER PRIMARY KEY ON CONFLICT REPLACE, v INTEGER)") -- Insert values and select them box.sql.execute("INSERT INTO t values (1, 1), (2, 2), (3, 1)") @@ -16,7 +16,7 @@ box.sql.execute("SELECT * FROM q") box.sql.execute("INSERT INTO p values (1, 1), (2, 2), (3, 1), (4, 5)") box.sql.execute("SELECT * FROM p") -box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (3, 1)") +box.sql.execute("INSERT INTO e values (1, 1), (2, 2), (1, 3)") box.sql.execute("SELECT * FROM e") box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY ON CONFLICT REPLACE)") -- 2.14.1