* [tarantool-patches] [PATCH] sql: allow ON CONFLICT REPLACE only for PK index
@ 2018-04-22 7:37 Bulat Niatshin
2018-05-03 23:39 ` [tarantool-patches] " n.pettik
0 siblings, 1 reply; 4+ messages in thread
From: Bulat Niatshin @ 2018-04-22 7:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, 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(
-- </conflict-6.4>
})
--- 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);
]], {
- -- <conflict-7.1>
- 1, 2, 3, 2, 3, 4
- -- </conflict-7.1>
+ 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);
]], {
- -- <conflict-7.2>
- 1, 2, 3, 2, 3, 4
- -- </conflict-7.2>
+ 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);
- ]], {
- -- <conflict-7.3>
- 1, "UNIQUE constraint failed: T1.C"
- -- </conflict-7.3>
- })
-
-test:do_execsql_test(
- "conflict-7.4",
- [[
- SELECT a,b,c FROM t1 ORDER BY a;
- ]], {
- -- <conflict-7.4>
- 1, 2, 3, 2, 3, 4, 4, 5, 6
- -- </conflict-7.4>
- })
-
--- 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;
- ]], {
- -- <conflict-8.1>
- 1, 2, 3, 2, 3, 4
- -- </conflict-8.1>
- })
-
--- 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;
- ]], {
- -- <conflict-8.2>
- 1, 2, 3, 2, 3, 4
- -- </conflict-8.2>
- })
-
--- 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);
- ]], {
- -- <conflict-8.3>
- 1, "UNIQUE constraint failed: T1.C"
- -- </conflict-8.3>
- })
-
-test:do_execsql_test(
- "conflict-8.4",
- [[
- SELECT a,b,c FROM t1 ORDER BY a;
- ]], {
- -- <conflict-8.4>
- 1, 2, 3, 2, 3, 4, 4, 5, 6
- -- </conflict-8.4>
- })
-
--- 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;
- ]], {
- -- <conflict-9.1>
- 1, 2, 3, 2, 3, 4
- -- </conflict-9.1>
- })
-
--- 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;
- ]], {
- -- <conflict-9.2>
- 1, 2, 3, 2, 3, 4
- -- </conflict-9.2>
- })
-
--- 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);
- ]], {
- -- <conflict-9.3>
- 1, "UNIQUE constraint failed: T1.C"
- -- </conflict-9.3>
- })
-
-test:do_execsql_test(
- "conflict-9.4",
- [[
- SELECT a,b,c FROM t1 ORDER BY a;
- ]], {
- -- <conflict-9.4>
- 1, 2, 3, 2, 3, 4, 4, 5, 6
- -- </conflict-9.4>
- })
-
--- 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);
]], {
- -- <conflict-10.1>
- 1, 2, 3, 2, 3, 4
- -- </conflict-10.1>
+ 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;
- ]], {
- -- <conflict-10.2>
- 1, 2, 3, 2, 3, 4
- -- </conflict-10.2>
- })
-
--- 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);
- ]], {
- -- <conflict-10.3>
- 1, "UNIQUE constraint failed: T1.C"
- -- </conflict-10.3>
- })
-
-test:do_execsql_test(
- "conflict-10.4",
- [[
- SELECT a,b,c FROM t1 ORDER BY a;
- ]], {
- -- <conflict-10.4>
- 1, 2, 3, 2, 3, 4, 4, 5, 6
- -- </conflict-10.4>
- })
-
--- 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;
- ]], {
- -- <conflict-11.1>
- 1, 2, 3, 2, 3, 4
- -- </conflict-11.1>
- })
-
--- 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);
]], {
- -- <conflict-11.2>
- 1, 2, 3, 2, 3, 4
- -- </conflict-11.2>
+ 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);
]], {
- -- <conflict-11.3>
- 1, "UNIQUE constraint failed: T1.C"
- -- </conflict-11.3>
+ 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;
- ]], {
- -- <conflict-11.4>
- 1, 2, 3, 2, 3, 4, 4, 5, 6
- -- </conflict-11.4>
- })
-
-
-
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [tarantool-patches] Re: [PATCH] sql: allow ON CONFLICT REPLACE only for PK index
2018-04-22 7:37 [tarantool-patches] [PATCH] sql: allow ON CONFLICT REPLACE only for PK index Bulat Niatshin
@ 2018-05-03 23:39 ` n.pettik
2018-05-11 6:01 ` Kirill Yukhin
0 siblings, 1 reply; 4+ messages in thread
From: n.pettik @ 2018-05-03 23:39 UTC (permalink / raw)
To: tarantool-patches
Cc: Кирилл Юхин
I am OK with this patch.
I have fixed a few minor codestyle violations and
rebased to fresh 2.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: allow ON CONFLICT REPLACE only for PK index
2018-05-03 23:39 ` [tarantool-patches] " n.pettik
@ 2018-05-11 6:01 ` Kirill Yukhin
2018-05-11 18:18 ` n.pettik
0 siblings, 1 reply; 4+ messages in thread
From: Kirill Yukhin @ 2018-05-11 6:01 UTC (permalink / raw)
To: n.pettik; +Cc: tarantool-patches
Hello Bulat, Nikita,
On 04 мая 02:39, n.pettik wrote:
> I am OK with this patch.
> I have fixed a few minor codestyle violations and
> rebased to fresh 2.0
The patch makes sql/on-conflict test fail.
Could you pls fix it.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: allow ON CONFLICT REPLACE only for PK index
2018-05-11 6:01 ` Kirill Yukhin
@ 2018-05-11 18:18 ` n.pettik
0 siblings, 0 replies; 4+ messages in thread
From: n.pettik @ 2018-05-11 18:18 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Yukhin
> On 11 May 2018, at 09:01, Kirill Yukhin <kyukhin@tarantool.org> wrote:
>
> Hello Bulat, Nikita,
> On 04 мая 02:39, n.pettik wrote:
>> I am OK with this patch.
>> I have fixed a few minor codestyle violations and
>> rebased to fresh 2.0
> The patch makes sql/on-conflict test fail.
> Could you pls fix it.
>
> --
> Regards, Kirill Yukhin
Well, I don’t have such troubles, everything works fine.
Are you sure that you are using last version?
Branch is bn/gh-2963-proper-replace
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-11 18:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-22 7:37 [tarantool-patches] [PATCH] sql: allow ON CONFLICT REPLACE only for PK index Bulat Niatshin
2018-05-03 23:39 ` [tarantool-patches] " n.pettik
2018-05-11 6:01 ` Kirill Yukhin
2018-05-11 18:18 ` n.pettik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox