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 40D132CC45 for ; Thu, 11 Oct 2018 09:20:29 -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 5vruwkJYsWQh for ; Thu, 11 Oct 2018 09:20:29 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5E6FB2CC38 for ; Thu, 11 Oct 2018 09:20:28 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: fix fk set null clause References: <20181009123750.3330-1-avkhatskevich@tarantool.org> From: Alex Khatskevich Message-ID: <1c7c98dc-99ee-1f2b-7996-83bdd4e1c6ee@tarantool.org> Date: Thu, 11 Oct 2018 16:20:25 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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" , tarantool-patches@freelists.org >> After changing behavior of the `IS` operator (#b3a3ddb571), >> `SET NULL` was rewritten to use `EQ` instead. Which do not respect > Nit: doesn’t. fixed >> NULLs. >> >> This commit fixes the null related behavior by emitting logical >> constructions equivalent for this case to old `IS`. >> Those constructions are not equal to the old `EQ` in common case. >> Before: >> `oldval` old_is `newval` >> Now: >> `oldval` is_null or (`newval` is_not_null and `oldval` eq `newval`) > I guess it is not completely correct: if oldval == NULL and newval != NULL, > than ‘before’ condition will be false, but ’now’ will be true. Did I miss smth? Yes. Those are equivalent for that particular case. I slightly updated the comment. New comment: ==== This commit fixes the null related behavior by emitting logical constructions equivalent for this case to old `IS`. The new expression works differently than old `IS` for nulls, however the difference doesn't change anything, because matched rows are then searched in a child table with `EQ` expression which do not match nulls. Before: `oldval` old_is `newval` Now: `oldval` is_null or (`newval` is_not_null and `oldval` eq `newval`) ===== >> Closes #3645 >> --- >> Issue: https://github.com/tarantool/tarantool/issues/3645 >> Branch: https://github.com/tarantool/tarantool/tree/kh/gh-3642-set-null >> >> src/box/sql/fkey.c | 49 ++++++++++++++------ >> test/sql-tap/gh3645-set-null.test.lua | 84 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 119 insertions(+), 14 deletions(-) >> create mode 100755 test/sql-tap/gh3645-set-null.test.lua >> >> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c >> index 091778fc8..0abf42c84 100644 >> --- a/src/box/sql/fkey.c >> +++ b/src/box/sql/fkey.c >> @@ -787,24 +787,45 @@ fkey_action_trigger(struct Parse *pParse, struct Table *pTab, struct fkey *fkey, >> >> /* >> * For ON UPDATE, construct the next term of the >> - * WHEN clause. The final WHEN clause will be like >> + * WHEN clause, which should return false in case >> + * there is a reason to for a broken constrant in >> + * a parent table: >> + * no_action_needed := `oldval` IS NULL OR >> + * (`newval` IS NOT NULL AND >> + * `newval` = `oldval`) >> + * >> + * The final WHEN clause will be like >> * this: >> * >> - * WHEN NOT(old.col1 = new.col1 AND ... AND >> - * old.colN = new.colN) >> + * WHEN NOT( no_action_needed(col1) AND ... >> + * no_action_needed(colN)) >> */ >> if (is_update) { >> - struct Expr *l, *r; >> - l = sqlite3PExpr(pParse, TK_DOT, >> - sqlite3ExprAlloc(db, TK_ID, &t_old, 0), >> - sqlite3ExprAlloc(db, TK_ID, &t_to_col, >> - 0)); >> - r = sqlite3PExpr(pParse, TK_DOT, >> - sqlite3ExprAlloc(db, TK_ID, &t_new, 0), >> - sqlite3ExprAlloc(db, TK_ID, &t_to_col, >> - 0)); >> - eq = sqlite3PExpr(pParse, TK_EQ, l, r); >> - when = sqlite3ExprAnd(db, when, eq); >> + Expr *old_val = sqlite3PExpr( > Nit: struct Expr *... > >> + pParse, TK_DOT, >> + sqlite3ExprAlloc(db, TK_ID, &t_old, 0), >> + sqlite3ExprAlloc(db, TK_ID, &t_to_col, >> + 0)); >> + Expr *newVal = sqlite3PExpr( > Nit: struct Expr new_val = … > And use everywhere else ‘struct' prefix. > >> + pParse, TK_DOT, >> + sqlite3ExprAlloc(db, TK_ID, &t_new, 0), >> + sqlite3ExprAlloc(db, TK_ID, &t_to_col, >> + 0)); >> + Expr *old_is_null = sqlite3PExpr( >> + pParse, TK_ISNULL, >> + sqlite3ExprDup(db, old_val, 0), NULL); >> + eq = sqlite3PExpr( >> + pParse, TK_EQ, >> + sqlite3ExprDup(db, old_val, 0), >> + sqlite3ExprDup(db, newVal, 0)); >> + Expr *new_non_null = sqlite3PExpr( >> + pParse, TK_NOTNULL, newVal, NULL); >> + Expr *non_null_eq = sqlite3PExpr( >> + pParse, TK_AND, new_non_null, eq); >> + Expr *no_action_needed = sqlite3PExpr( >> + pParse, TK_OR, old_is_null, >> + non_null_eq); >> + when = sqlite3ExprAnd(db, when, no_action_needed); > How memory for old_val will be released? It’s twice dupped, but original > value will be not freed. new_val is once dupped and once used, for example. > I guess it looks like leak. Yes, it is. > > Overall, I suggest cosmetic diff (indentation + struct prefixes): > > +++ b/src/box/sql/fkey.c > @@ -801,30 +801,24 @@ fkey_action_trigger(struct Parse *pParse, struct Table *pTab, struct fkey *fkey, > * no_action_needed(colN)) > */ > if (is_update) { > - Expr *old_val = sqlite3PExpr( > - pParse, TK_DOT, > + struct Expr *old_val = sqlite3PExpr(pParse, TK_DOT, > sqlite3ExprAlloc(db, TK_ID, &t_old, 0), > - sqlite3ExprAlloc(db, TK_ID, &t_to_col, > - 0)); > - Expr *newVal = sqlite3PExpr( > - pParse, TK_DOT, > + sqlite3ExprAlloc(db, TK_ID, &t_to_col, 0)); > + struct Expr *new_val = sqlite3PExpr(pParse, TK_DOT, > sqlite3ExprAlloc(db, TK_ID, &t_new, 0), > - sqlite3ExprAlloc(db, TK_ID, &t_to_col, > - 0)); > - Expr *old_is_null = sqlite3PExpr( > - pParse, TK_ISNULL, > + sqlite3ExprAlloc(db, TK_ID, &t_to_col, 0)); > + struct Expr *old_is_null = sqlite3PExpr(pParse, TK_ISNULL, > sqlite3ExprDup(db, old_val, 0), NULL); > - eq = sqlite3PExpr( > - pParse, TK_EQ, > - sqlite3ExprDup(db, old_val, 0), > - sqlite3ExprDup(db, newVal, 0)); > - Expr *new_non_null = sqlite3PExpr( > - pParse, TK_NOTNULL, newVal, NULL); > - Expr *non_null_eq = sqlite3PExpr( > - pParse, TK_AND, new_non_null, eq); > - Expr *no_action_needed = sqlite3PExpr( > - pParse, TK_OR, old_is_null, > - non_null_eq); > + eq = sqlite3PExpr(pParse, TK_EQ, > + sqlite3ExprDup(db, old_val, 0), > + sqlite3ExprDup(db, new_val, 0)); > + struct Expr *new_non_null = > + sqlite3PExpr(pParse, TK_NOTNULL, new_val, NULL); > + struct Expr *non_null_eq = > + sqlite3PExpr(pParse, TK_AND, new_non_null, eq); > + struct Expr *no_action_needed = > + sqlite3PExpr(pParse, TK_OR, old_is_null, > + non_null_eq); applied. >> } >> >> if (action != FKEY_ACTION_RESTRICT && >> diff --git a/test/sql-tap/gh3645-set-null.test.lua b/test/sql-tap/gh3645-set-null.test.lua >> new file mode 100755 >> index 000000000..9067c52f1 >> --- /dev/null >> +++ b/test/sql-tap/gh3645-set-null.test.lua > I wouldn’t create separate test-file for this ticket: you could > use one of fkey1-4 test suites. moved > >> @@ -0,0 +1,84 @@ >> +#!/usr/bin/env tarantool >> +test = require("sqltester") >> +test:plan(11) >> + >> +test:do_catchsql_test( >> + "set-null-1.0", >> + [[ >> + CREATE TABLE T1 (a INTEGER PRIMARY KEY, >> + b char(5) unique); >> + CREATE TABLE T2 (a INTEGER PRIMARY KEY, >> + b char(5) unique, >> + foreign key (b) references t1 (b) on update set null); > Nit: use uppercase for SQL keywords. done > >> + INSERT INTO T1 VALUES (1,'a'); >> + INSERT INTO T2 VALUES (1,'a’); > Nit: indentation is broken. fixed > >> + ]], {0}); >> + >> +test:do_catchsql_test( >> + "set-null-1.1", >> + [[ >> + UPDATE T1 SET B = NULL; >> + ]], {0}); >> + >> +test:do_catchsql_test( > Why do you use catchsql? I guess execsql would be more suitable here > and for the rest tests as well (except for UPDATE statements). fixed > >> +test:do_catchsql_test( >> + "set-null-2.0", >> + [[ >> + CREATE TABLE T3 (a INTEGER PRIMARY KEY, >> + b char(5) unique); >> + CREATE TABLE T4 (a INTEGER PRIMARY KEY, >> + b char(5) unique, >> + foreign key (b) references t3 (b) on update cascade); >> + INSERT INTO T3 VALUES (1,'a'); >> + INSERT INTO T4 VALUES (1,'a'); >> + ]], {0}); > Indentation is broken. > How ‘ON UPDATE CASCADE’ clause is related to ’SET NULL’? Not related. Similar test exists. Deleted. New diff: commit 77d76c1c664f992efc54b5a3329a44a185b1e4cf Author: AKhatskevich Date:   Mon Oct 8 19:26:59 2018 +0300     sql: fix fk set null clause     After changing behavior of the `IS` operator (#b3a3ddb571),     `SET NULL` was rewritten to use `EQ` instead. Which doesn't respect     NULLs.     This commit fixes the null related behavior by emitting logical     constructions equivalent for this case to old `IS`.     The new expression works differently than old `IS` for nulls, however     the difference doesn't change anything, because matched rows are then     searched in a child table with `EQ` expression which do not match nulls.     Before:     `oldval` old_is `newval`     Now:     `oldval` is_null or (`newval` is_not_null and `oldval` eq `newval`)     Closes #3645 diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 091778fc8..56885e448 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -787,24 +787,39 @@ fkey_action_trigger(struct Parse *pParse, struct Table *pTab, struct fkey *fkey,          /*           * For ON UPDATE, construct the next term of the -         * WHEN clause. The final WHEN clause will be like +         * WHEN clause, which should return false in case +         * there is a reason to for a broken constrant in +         * a parent table: +         *     no_action_needed := `oldval` IS NULL OR +         *         (`newval` IS NOT NULL AND +         *             `newval` = `oldval`) +         * +         * The final WHEN clause will be like           * this:           * -         *    WHEN NOT(old.col1 = new.col1 AND ... AND -         *             old.colN = new.colN) +         *    WHEN NOT( no_action_needed(col1) AND ... +         *        no_action_needed(colN))           */          if (is_update) { -            struct Expr *l, *r; -            l = sqlite3PExpr(pParse, TK_DOT, -                     sqlite3ExprAlloc(db, TK_ID, &t_old, 0), -                     sqlite3ExprAlloc(db, TK_ID, &t_to_col, -                              0)); -            r = sqlite3PExpr(pParse, TK_DOT, -                     sqlite3ExprAlloc(db, TK_ID, &t_new, 0), -                     sqlite3ExprAlloc(db, TK_ID, &t_to_col, -                              0)); -            eq = sqlite3PExpr(pParse, TK_EQ, l, r); -            when = sqlite3ExprAnd(db, when, eq); +            struct Expr *old_val = sqlite3PExpr(pParse, TK_DOT, +                sqlite3ExprAlloc(db, TK_ID, &t_old, 0), +                sqlite3ExprAlloc(db, TK_ID, &t_to_col, 0)); +            struct Expr *new_val = sqlite3PExpr(pParse, TK_DOT, +                sqlite3ExprAlloc(db, TK_ID, &t_new, 0), +                sqlite3ExprAlloc(db, TK_ID, &t_to_col, 0)); +            struct Expr *old_is_null = sqlite3PExpr( +                pParse, TK_ISNULL, +                sqlite3ExprDup(db, old_val, 0), NULL); +            eq = sqlite3PExpr(pParse, TK_EQ, old_val, +                sqlite3ExprDup(db, new_val, 0)); +            struct Expr *new_non_null = +                sqlite3PExpr(pParse, TK_NOTNULL, new_val, NULL); +            struct Expr *non_null_eq = +                sqlite3PExpr(pParse, TK_AND, new_non_null, eq); +            struct Expr *no_action_needed = +                sqlite3PExpr(pParse, TK_OR, old_is_null, +                         non_null_eq); +            when = sqlite3ExprAnd(db, when, no_action_needed);          }          if (action != FKEY_ACTION_RESTRICT && diff --git a/test/sql-tap/fkey1.test.lua b/test/sql-tap/fkey1.test.lua index 3c29b097d..b419e4d59 100755 --- a/test/sql-tap/fkey1.test.lua +++ b/test/sql-tap/fkey1.test.lua @@ -1,6 +1,6 @@  #!/usr/bin/env tarantool  test = require("sqltester") -test:plan(18) +test:plan(25)  -- This file implements regression tests for foreign keys. @@ -233,4 +233,27 @@ test:do_execsql_test(          --      }) +-- gh-3645: update col=Null do not activates ON UPDATE trigger. + +test:do_select_tests( +    "fkey1-7", +    { +        {"0", +            [[ +                CREATE TABLE T12 (A INTEGER PRIMARY KEY, +                    B CHAR(5) UNIQUE); +                CREATE TABLE T13 (A INTEGER PRIMARY KEY, +                    B CHAR(5) UNIQUE, +                    FOREIGN KEY (B) REFERENCES T12 (B) ON UPDATE SET NULL); +                INSERT INTO T12 VALUES (1,'a'); +                INSERT INTO T13 VALUES (1,'a'); +            ]], {}}, +        {"1", "UPDATE T12 SET B = NULL", {}}, +        {"2", "SELECT * FROM T12", {1, ""}}, +        {"3", "SELECT * FROM T13", {1, ""}}, +        {"4", "UPDATE T12 SET B = 'a'", {}}, +        {"5", "SELECT * FROM T12", {1, "a"}}, +        {"6", "SELECT * FROM T13", {1, ""}}, +    }) +  test:finish_test()