* [tarantool-patches] [PATCH] sql: fix fk set null clause @ 2018-10-09 12:37 AKhatskevich 2018-10-09 19:14 ` [tarantool-patches] " n.pettik 2018-10-26 5:47 ` Kirill Yukhin 0 siblings, 2 replies; 5+ messages in thread From: AKhatskevich @ 2018-10-09 12:37 UTC (permalink / raw) To: korablev, tarantool-patches After changing behavior of the `IS` operator (#b3a3ddb571), `SET NULL` was rewritten to use `EQ` instead. Which do not respect 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`) 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( + 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_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); } 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 @@ -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); + INSERT INTO T1 VALUES (1,'a'); + INSERT INTO T2 VALUES (1,'a'); + ]], {0}); + +test:do_catchsql_test( + "set-null-1.1", + [[ + UPDATE T1 SET B = NULL; + ]], {0}); + +test:do_catchsql_test( + "set-null-1.2", + [[ + SELECT * FROM T1; + ]], {0, {1, ""}}); + +test:do_catchsql_test( + "set-null-1.3", + [[ + SELECT * FROM T2; + ]], {0, {1, ""}}); + +test:do_catchsql_test( + "set-null-1.4", + [[ + UPDATE T1 SET B = 'a'; + ]], {0}); + +test:do_catchsql_test( + "set-null-1.5", + [[ + SELECT * FROM T1; + ]], {0, {1, "a"}}); + +test:do_catchsql_test( + "set-null-1.6", + [[ + SELECT * FROM T2; + ]], {0, {1, ""}}); + + +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}); + +test:do_catchsql_test( + "set-null-2.1", + [[ + UPDATE T3 SET B = 'b'; + ]], {0}); + +test:do_catchsql_test( + "set-null-2.2", + [[ + SELECT * FROM T3; + ]], {0, {1, "b"}}); + +test:do_catchsql_test( + "set-null-2.3", + [[ + SELECT * FROM T4; + ]], {0, {1, "b"}}); + +test:finish_test() -- 2.14.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix fk set null clause 2018-10-09 12:37 [tarantool-patches] [PATCH] sql: fix fk set null clause AKhatskevich @ 2018-10-09 19:14 ` n.pettik 2018-10-11 13:20 ` Alex Khatskevich 2018-10-26 5:47 ` Kirill Yukhin 1 sibling, 1 reply; 5+ messages in thread From: n.pettik @ 2018-10-09 19:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich > After changing behavior of the `IS` operator (#b3a3ddb571), > `SET NULL` was rewritten to use `EQ` instead. Which do not respect Nit: doesn’t. > 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? > > 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. 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); > } > > 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. > @@ -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. > + INSERT INTO T1 VALUES (1,'a'); > + INSERT INTO T2 VALUES (1,'a’); Nit: indentation is broken. > + ]], {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). > +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’? ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix fk set null clause 2018-10-09 19:14 ` [tarantool-patches] " n.pettik @ 2018-10-11 13:20 ` Alex Khatskevich 2018-10-17 15:41 ` n.pettik 0 siblings, 1 reply; 5+ messages in thread From: Alex Khatskevich @ 2018-10-11 13:20 UTC (permalink / raw) To: n.pettik, tarantool-patches >> 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 <avkhatskevich@tarantool.org> 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( -- </fkey1-6.3> }) +-- 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() ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix fk set null clause 2018-10-11 13:20 ` Alex Khatskevich @ 2018-10-17 15:41 ` n.pettik 0 siblings, 0 replies; 5+ messages in thread From: n.pettik @ 2018-10-17 15:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Alex Khatskevich, Kirill Yukhin LGTM ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix fk set null clause 2018-10-09 12:37 [tarantool-patches] [PATCH] sql: fix fk set null clause AKhatskevich 2018-10-09 19:14 ` [tarantool-patches] " n.pettik @ 2018-10-26 5:47 ` Kirill Yukhin 1 sibling, 0 replies; 5+ messages in thread From: Kirill Yukhin @ 2018-10-26 5:47 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev Hello, On 09 Oct 15:37, AKhatskevich wrote: > After changing behavior of the `IS` operator (#b3a3ddb571), > `SET NULL` was rewritten to use `EQ` instead. Which do not respect > 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`) > > 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 I've pushed the patch to 2.1 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-26 5:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-09 12:37 [tarantool-patches] [PATCH] sql: fix fk set null clause AKhatskevich 2018-10-09 19:14 ` [tarantool-patches] " n.pettik 2018-10-11 13:20 ` Alex Khatskevich 2018-10-17 15:41 ` n.pettik 2018-10-26 5:47 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox