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 813572BDA7 for ; Tue, 9 Oct 2018 15:14:40 -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 QJQO0PrdJq1w for ; Tue, 9 Oct 2018 15:14:40 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 2FA4927642 for ; Tue, 9 Oct 2018 15:14:40 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: fix fk set null clause From: "n.pettik" In-Reply-To: <20181009123750.3330-1-avkhatskevich@tarantool.org> Date: Tue, 9 Oct 2018 22:14:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20181009123750.3330-1-avkhatskevich@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: tarantool-patches@freelists.org 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=E2=80=99t. > NULLs. >=20 > 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 =3D=3D NULL and newval = !=3D NULL, than =E2=80=98before=E2=80=99 condition will be false, but =E2=80=99now=E2= =80=99 will be true. Did I miss smth? >=20 > Closes #3645 > --- > Issue: https://github.com/tarantool/tarantool/issues/3645 > Branch: = https://github.com/tarantool/tarantool/tree/kh/gh-3642-set-null >=20 > 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 >=20 > 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, >=20 > /* > * 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 :=3D `oldval` IS NULL OR > + * (`newval` IS NOT NULL AND > + * `newval` =3D `oldval`) > + * > + * The final WHEN clause will be like > * this: > * > - * WHEN NOT(old.col1 =3D new.col1 AND ... AND > - * old.colN =3D new.colN) > + * WHEN NOT( no_action_needed(col1) AND ... > + * no_action_needed(colN)) > */ > if (is_update) { > - struct Expr *l, *r; > - l =3D sqlite3PExpr(pParse, TK_DOT, > - sqlite3ExprAlloc(db, TK_ID, = &t_old, 0), > - sqlite3ExprAlloc(db, TK_ID, = &t_to_col, > - 0)); > - r =3D sqlite3PExpr(pParse, TK_DOT, > - sqlite3ExprAlloc(db, TK_ID, = &t_new, 0), > - sqlite3ExprAlloc(db, TK_ID, = &t_to_col, > - 0)); > - eq =3D sqlite3PExpr(pParse, TK_EQ, l, r); > - when =3D sqlite3ExprAnd(db, when, eq); > + Expr *old_val =3D sqlite3PExpr( Nit: struct Expr *... > + pParse, TK_DOT, > + sqlite3ExprAlloc(db, TK_ID, &t_old, 0), > + sqlite3ExprAlloc(db, TK_ID, &t_to_col, > + 0)); > + Expr *newVal =3D sqlite3PExpr( Nit: struct Expr new_val =3D =E2=80=A6 And use everywhere else =E2=80=98struct' prefix. > + pParse, TK_DOT, > + sqlite3ExprAlloc(db, TK_ID, &t_new, 0), > + sqlite3ExprAlloc(db, TK_ID, &t_to_col, > + 0)); > + Expr *old_is_null =3D sqlite3PExpr( > + pParse, TK_ISNULL, > + sqlite3ExprDup(db, old_val, 0), NULL); > + eq =3D sqlite3PExpr( > + pParse, TK_EQ, > + sqlite3ExprDup(db, old_val, 0), > + sqlite3ExprDup(db, newVal, 0)); > + Expr *new_non_null =3D sqlite3PExpr( > + pParse, TK_NOTNULL, newVal, NULL); > + Expr *non_null_eq =3D sqlite3PExpr( > + pParse, TK_AND, new_non_null, eq); > + Expr *no_action_needed =3D sqlite3PExpr( > + pParse, TK_OR, old_is_null, > + non_null_eq); > + when =3D sqlite3ExprAnd(db, when, = no_action_needed); How memory for old_val will be released? It=E2=80=99s 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 =3D sqlite3PExpr( - pParse, TK_DOT, + struct Expr *old_val =3D sqlite3PExpr(pParse, = TK_DOT, sqlite3ExprAlloc(db, TK_ID, &t_old, 0), - sqlite3ExprAlloc(db, TK_ID, &t_to_col, - 0)); - Expr *newVal =3D sqlite3PExpr( - pParse, TK_DOT, + sqlite3ExprAlloc(db, TK_ID, &t_to_col, = 0)); + struct Expr *new_val =3D sqlite3PExpr(pParse, = TK_DOT, sqlite3ExprAlloc(db, TK_ID, &t_new, 0), - sqlite3ExprAlloc(db, TK_ID, &t_to_col, - 0)); - Expr *old_is_null =3D sqlite3PExpr( - pParse, TK_ISNULL, + sqlite3ExprAlloc(db, TK_ID, &t_to_col, = 0)); + struct Expr *old_is_null =3D = sqlite3PExpr(pParse, TK_ISNULL, sqlite3ExprDup(db, old_val, 0), NULL); - eq =3D sqlite3PExpr( - pParse, TK_EQ, - sqlite3ExprDup(db, old_val, 0), - sqlite3ExprDup(db, newVal, 0)); - Expr *new_non_null =3D sqlite3PExpr( - pParse, TK_NOTNULL, newVal, NULL); - Expr *non_null_eq =3D sqlite3PExpr( - pParse, TK_AND, new_non_null, eq); - Expr *no_action_needed =3D sqlite3PExpr( - pParse, TK_OR, old_is_null, - non_null_eq); + eq =3D sqlite3PExpr(pParse, TK_EQ, + sqlite3ExprDup(db, old_val, = 0), + sqlite3ExprDup(db, new_val, = 0)); + struct Expr *new_non_null =3D + sqlite3PExpr(pParse, TK_NOTNULL, = new_val, NULL); + struct Expr *non_null_eq =3D + sqlite3PExpr(pParse, TK_AND, = new_non_null, eq); + struct Expr *no_action_needed =3D + sqlite3PExpr(pParse, TK_OR, old_is_null, + non_null_eq); > } >=20 > if (action !=3D 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=E2=80=99t 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 =3D 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=E2=80=99); Nit: indentation is broken. > + ]], {0}); > + > +test:do_catchsql_test( > + "set-null-1.1", > + [[ > + UPDATE T1 SET B =3D 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 =E2=80=98ON UPDATE CASCADE=E2=80=99 clause is related to =E2=80=99SET = NULL=E2=80=99?