[tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause

[n.pettik]

> On 13 Feb 2019, at 09:08, Kirill Yukhin <kyukhin at 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.