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 56D642768F for ; Thu, 14 Feb 2019 03:56:44 -0500 (EST) 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 wN0H1RF7Ab32 for ; Thu, 14 Feb 2019 03:56:44 -0500 (EST) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 7C37D27685 for ; Thu, 14 Feb 2019 03:56:43 -0500 (EST) Date: Thu, 14 Feb 2019 11:56:40 +0300 From: Kirill Yukhin Subject: [tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause Message-ID: <20190214085640.5fkerrtzxfc5mluu@tarantool.org> References: <68664d5ad6be9959c4364a288ccc2feed84cd8b4.1550037810.git.kyukhin@tarantool.org> <50B90E82-7ECC-4C12-B7EF-132E77EDDDEC@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <50B90E82-7ECC-4C12-B7EF-132E77EDDDEC@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: "n.pettik" Cc: tarantool-patches@freelists.org Hello Nikita, On 13 Feb 22:43, n.pettik wrote: > > > On 13 Feb 2019, at 09:08, Kirill Yukhin 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")