* [tarantool-patches] [PATCH] sql: prohibit duplication of FK action clause @ 2019-02-13 6:08 Kirill Yukhin 2019-02-13 19:43 ` [tarantool-patches] " n.pettik 0 siblings, 1 reply; 4+ messages in thread From: Kirill Yukhin @ 2019-02-13 6:08 UTC (permalink / raw) To: korablev; +Cc: tarantool-patches, Kirill Yukhin The patch prohibits duplication of action clause for foreign key constraints. It is now syntax error to specify, say, ON UPDATE clause for the same FK twice. The patch also remove ON INSERT caluse at all, since it ultimately is no-op and leads to nothing but confusion. Closes #3475 --- https://github.com/tarantool/tarantool/issues/3475 https://github.com/tarantool/tarantool/commits/kyukhin/gh-3475-fk-duplicate-action-clause src/box/sql/parse.y | 10 +++++++-- test/sql/gh-3475-fk-duplicate-clause.result | 32 +++++++++++++++++++++++++++ test/sql/gh-3475-fk-duplicate-clause.test.lua | 13 +++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 test/sql/gh-3475-fk-duplicate-clause.result create mode 100644 test/sql/gh-3475-fk-duplicate-clause.test.lua diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 32ef685..7938afd 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -283,10 +283,16 @@ autoinc(X) ::= AUTOINCR. {X = 1;} // %type refargs {int} refargs(A) ::= . { A = FKEY_NO_ACTION; } -refargs(A) ::= refargs(A) refarg(Y). { A = (A & ~Y.mask) | Y.value; } +refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { + A = (A & ~Y.mask) | Y.value; + } else { + sqlite3ErrorMsg(pParse, + "near \"%T\": duplicate FK action clause.", + &pParse->sLastToken); + } + } %type refarg {struct {int value; int mask;}} refarg(A) ::= MATCH matcharg(X). { A.value = X<<16; A.mask = 0xff0000; } -refarg(A) ::= ON INSERT refact. { A.value = 0; A.mask = 0x000000; } refarg(A) ::= ON DELETE refact(X). { A.value = X; A.mask = 0x0000ff; } refarg(A) ::= ON UPDATE refact(X). { A.value = X<<8; A.mask = 0x00ff00; } %type matcharg {int} diff --git a/test/sql/gh-3475-fk-duplicate-clause.result b/test/sql/gh-3475-fk-duplicate-clause.result new file mode 100644 index 0000000..7bcd62b --- /dev/null +++ b/test/sql/gh-3475-fk-duplicate-clause.result @@ -0,0 +1,32 @@ +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +box.cfg{} +--- +... +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)") +--- +... +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)") +--- +- error: 'near ")": duplicate FK action clause.' +... +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)") +--- +- error: 'near ")": duplicate FK action clause.' +... +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)") +--- +... +box.sql.execute("DROP TABLE t2") +--- +... +box.sql.execute("DROP TABLE t1") +--- +... diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua new file mode 100644 index 0000000..447c104 --- /dev/null +++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua @@ -0,0 +1,13 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +box.cfg{} + +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)") + +box.sql.execute("DROP TABLE t2") +box.sql.execute("DROP TABLE t1") -- 2.11.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause 2019-02-13 6:08 [tarantool-patches] [PATCH] sql: prohibit duplication of FK action clause Kirill Yukhin @ 2019-02-13 19:43 ` n.pettik 2019-02-14 8:56 ` Kirill Yukhin 0 siblings, 1 reply; 4+ messages in thread From: n.pettik @ 2019-02-13 19:43 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin > On 13 Feb 2019, at 09:08, Kirill Yukhin <kyukhin@tarantool.org> wrote: > > The patch prohibits duplication of action clause for foreign > key constraints. It is now syntax error to specify, say, > ON UPDATE clause for the same FK twice. > The patch also remove ON INSERT caluse at all, since it > ultimately is no-op and leads to nothing but confusion. > > Closes #3475 > --- > https://github.com/tarantool/tarantool/issues/3475 > https://github.com/tarantool/tarantool/commits/kyukhin/gh-3475-fk-duplicate-action-clause Travis status is completely negative. Please, make sure it is green before sending patch. > src/box/sql/parse.y | 10 +++++++-- > test/sql/gh-3475-fk-duplicate-clause.result | 32 +++++++++++++++++++++++++++ > test/sql/gh-3475-fk-duplicate-clause.test.lua | 13 +++++++++++ > 3 files changed, 53 insertions(+), 2 deletions(-) > create mode 100644 test/sql/gh-3475-fk-duplicate-clause.result > create mode 100644 test/sql/gh-3475-fk-duplicate-clause.test.lua > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index 32ef685..7938afd 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -283,10 +283,16 @@ autoinc(X) ::= AUTOINCR. {X = 1;} > // > %type refargs {int} > refargs(A) ::= . { A = FKEY_NO_ACTION; } > -refargs(A) ::= refargs(A) refarg(Y). { A = (A & ~Y.mask) | Y.value; } > +refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { > + A = (A & ~Y.mask) | Y.value; > + } else { > + sqlite3ErrorMsg(pParse, > + "near \"%T\": duplicate FK action clause.", > + &pParse->sLastToken); > + } > + } Please, fix indentation, it looks extremely awful. Lets use 2 spaces as a default indent (as almost everywhere in parse.y). > diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua > new file mode 100644 > index 0000000..447c104 > --- /dev/null > +++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua > @@ -0,0 +1,13 @@ > +test_run = require('test_run').new() > +engine = test_run:get_cfg('engine') > +box.sql.execute('pragma sql_default_engine=\''..engine..'\’') Should this test be engine-dependent? > + > +box.cfg{} > + > +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)") > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)") > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)") > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)”) What about next cases: box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)”) box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )”) Sorry, can’t check myself since can’t build your branch. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause 2019-02-13 19:43 ` [tarantool-patches] " n.pettik @ 2019-02-14 8:56 ` Kirill Yukhin 2019-02-14 13:44 ` n.pettik 0 siblings, 1 reply; 4+ messages in thread From: Kirill Yukhin @ 2019-02-14 8:56 UTC (permalink / raw) To: n.pettik; +Cc: tarantool-patches Hello Nikita, On 13 Feb 22:43, n.pettik wrote: > > > On 13 Feb 2019, at 09:08, Kirill Yukhin <kyukhin@tarantool.org> wrote: > > > > The patch prohibits duplication of action clause for foreign > > key constraints. It is now syntax error to specify, say, > > ON UPDATE clause for the same FK twice. > > The patch also remove ON INSERT caluse at all, since it > > ultimately is no-op and leads to nothing but confusion. > > > > Closes #3475 > > --- > > https://github.com/tarantool/tarantool/issues/3475 > > https://github.com/tarantool/tarantool/commits/kyukhin/gh-3475-fk-duplicate-action-clause > > Travis status is completely negative. Please, make sure it is green > before sending patch. Travis is completely positive w/ this patch. Please, see [1]. Failing case is the branch after rebase. Anyway, rebased version is fixed as well. > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > > index 32ef685..7938afd 100644 > > --- a/src/box/sql/parse.y > > +++ b/src/box/sql/parse.y > > @@ -283,10 +283,16 @@ autoinc(X) ::= AUTOINCR. {X = 1;} > > // > > %type refargs {int} > > refargs(A) ::= . { A = FKEY_NO_ACTION; } > > -refargs(A) ::= refargs(A) refarg(Y). { A = (A & ~Y.mask) | Y.value; } > > +refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { > > + A = (A & ~Y.mask) | Y.value; > > + } else { > > + sqlite3ErrorMsg(pParse, > > + "near \"%T\": duplicate FK action clause.", > > + &pParse->sLastToken); > > + } > > + } > > Please, fix indentation, it looks extremely awful. > Lets use 2 spaces as a default indent (as almost > everywhere in parse.y). I've replaced 4 spaces w/ 2 for tab. > > diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua > > new file mode 100644 > > index 0000000..447c104 > > --- /dev/null > > +++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua > > @@ -0,0 +1,13 @@ > > +test_run = require('test_run').new() > > +engine = test_run:get_cfg('engine') > > +box.sql.execute('pragma sql_default_engine=\''..engine..'\’') > > Should this test be engine-dependent? I'd test everything on boath engines. But you're reviewer, so removed. > > + > > +box.cfg{} > > + > > +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)") > > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)") > > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)") > > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)”) > > What about next cases: > > box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)”) > box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )”) > > Sorry, can’t check myself since can’t build your branch. I've added them as well. [1] - https://travis-ci.org/tarantool/tarantool/builds/492546000 -- Regards, Kirill Yukhin diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 8c8fdc2..7ee8dc2 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -284,11 +284,11 @@ autoinc(X) ::= AUTOINCR. {X = 1;} %type refargs {int} refargs(A) ::= . { A = FKEY_NO_ACTION; } refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { - A = (A & ~Y.mask) | Y.value; + A = (A & ~Y.mask) | Y.value; } else { - sqlite3ErrorMsg(pParse, - "near \"%T\": duplicate FK action clause.", - &pParse->sLastToken); + sqlErrorMsg(pParse, + "near \"%T\": duplicate FK action clause.", + &pParse->sLastToken); } } %type refarg {struct {int value; int mask;}} diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg index 0007d8d..4ec39ac 100644 --- a/test/sql/engine.cfg +++ b/test/sql/engine.cfg @@ -1,4 +1,5 @@ { + "gh-3475-fk-duplicate-clause.test.lua": {}, "*": { "memtx": {"engine": "memtx"}, "vinyl": {"engine": "vinyl"} diff --git a/test/sql/gh-3475-fk-duplicate-clause.result b/test/sql/gh-3475-fk-duplicate-clause.result index 7bcd62b..5ce556f 100644 --- a/test/sql/gh-3475-fk-duplicate-clause.result +++ b/test/sql/gh-3475-fk-duplicate-clause.result @@ -1,12 +1,6 @@ test_run = require('test_run').new() --- ... -engine = test_run:get_cfg('engine') ---- -... -box.sql.execute('pragma sql_default_engine=\''..engine..'\'') ---- -... box.cfg{} --- ... @@ -21,6 +15,14 @@ box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DE --- - error: 'near ")": duplicate FK action clause.' ... +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)") +--- +- error: 'near ")": duplicate FK action clause.' +... +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )") +--- +- error: 'near ")": duplicate FK action clause.' +... box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)") --- ... diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua index 447c104..2c2b55c 100644 --- a/test/sql/gh-3475-fk-duplicate-clause.test.lua +++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua @@ -1,12 +1,12 @@ test_run = require('test_run').new() -engine = test_run:get_cfg('engine') -box.sql.execute('pragma sql_default_engine=\''..engine..'\'') box.cfg{} box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)") box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)") box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)") +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )") box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)") box.sql.execute("DROP TABLE t2") ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause 2019-02-14 8:56 ` Kirill Yukhin @ 2019-02-14 13:44 ` n.pettik 0 siblings, 0 replies; 4+ messages in thread From: n.pettik @ 2019-02-14 13:44 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Yukhin > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index 8c8fdc2..7ee8dc2 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -284,11 +284,11 @@ autoinc(X) ::= AUTOINCR. {X = 1;} > %type refargs {int} > refargs(A) ::= . { A = FKEY_NO_ACTION; } > refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { > - A = (A & ~Y.mask) | Y.value; > + A = (A & ~Y.mask) | Y.value; > } else { > - sqlite3ErrorMsg(pParse, > - "near \"%T\": duplicate FK action clause.", > - &pParse->sLastToken); > + sqlErrorMsg(pParse, > + "near \"%T\": duplicate FK action clause.", > + &pParse->sLastToken); > } > } Doesn’t look better. What I really meant is: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 7ee8dc292..cc4afaf71 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -283,14 +283,14 @@ autoinc(X) ::= AUTOINCR. {X = 1;} // %type refargs {int} refargs(A) ::= . { A = FKEY_NO_ACTION; } -refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) { - A = (A & ~Y.mask) | Y.value; - } else { - sqlErrorMsg(pParse, - "near \"%T\": duplicate FK action clause.", - &pParse->sLastToken); - } - } +refargs(A) ::= refargs(A) refarg(Y). { + if ((A & Y.mask) == 0) { + A = (A & ~Y.mask) | Y.value; + } else { + sqlErrorMsg(pParse, "near \"%T\": duplicate FK action clause.", + &pParse->sLastToken); + } +} What is more, I found this: tarantool> box.sql.execute("CREATE TABLE t3(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE NO ACTION ON DELETE NO ACTION)”) Is executed correctly. The same for MATCH: tarantool> box.sql.execute("CREATE TABLE t4(a INT PRIMARY KEY, b INT REFERENCES t1(b) MATCH SIMPLE ON DELETE NO ACTION ON UPDATE SET NULL MATCH FULL)”) Obviously, it happens since NO ACTION and SIMPLE are represented as zero valued members of enum. I don’t like idea to start enum from value 1. Firstly, according to ANSI grammar, MATCH clause always comes before triggered actions: <references specification> ::= REFERENCES <referenced table and columns> [ MATCH <match type> ] [ <referential triggered action> ] So, we can simplify grammar to smth like this: refagrs(A) :: = matcharg(X) refarg(Y) refarg(Z) . // Note that refargs is no longer recursive rule. Then, we can got rid of mask at all: %type refarg {struct {int value; bool is_delete_action_set; bool is_update_action_set ;}} refarg(A) :: = ON DELETE reface(X). { if(A.is_delete_action_set) //raise an error A.is_delete_action_set = true; A.value = X; } refarg(A) :: = ON UPDATE reface(X). {A.is_update_action_set = true; A.value = X; } ... That is my suggestion. I didn’t test it, but idea seems to be valid. Alternatively, you can come up with better solution. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-14 13:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-13 6:08 [tarantool-patches] [PATCH] sql: prohibit duplication of FK action clause Kirill Yukhin 2019-02-13 19:43 ` [tarantool-patches] " n.pettik 2019-02-14 8:56 ` Kirill Yukhin 2019-02-14 13:44 ` n.pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox