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 E9F5C28AFC for ; Tue, 4 Sep 2018 07:00:08 -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 gx3b3Zi9bTCo for ; Tue, 4 Sep 2018 07:00:08 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 4D917286AA for ; Tue, 4 Sep 2018 07:00:08 -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 v1 3/3] sql: dissallow bindings for DDL From: "n.pettik" In-Reply-To: <5049d3e7b70b7091c51ac99fc64f14a07c879c8a.1535730218.git.kshcherbatov@tarantool.org> Date: Tue, 4 Sep 2018 14:00:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <5049d3e7b70b7091c51ac99fc64f14a07c879c8a.1535730218.git.kshcherbatov@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: Kirill Shcherbatov Previous two patches looks OK to me. > Bindings could not be used in stored ACTs because they allocate Nit: =E2=80=9CAST=E2=80=9D. > memory registers and makes Nit: =E2=80=9Cmake=E2=80=9D (or better =E2=80=9Cprocess"). > assignments on parse sequentially. Nit: =E2=80=9Cduring parsing=E2=80=9D. So? Describe bug in details pls - it could help reviewer as well as those who will resurrect some day these bindings. Also, I guess problem is quite more sophisticated as it seems: a) For check constraint bindings really make no sense (I can=E2=80=99t come up with example how they can be used there at = all). b) For triggers currently we don=E2=80=99t have proper mechanism, which would allow to use bindings. Original SQLite also lacks it. To reuse the same trigger=E2=80=99s body with different parameters we = should not only be able to store it as prepared statement and substitute = literals, but also give trigger new name.=20 > Original sqlite3 did validations that persistent AST doesn't have > auto-assigment Varibles on triggers and checks creation. > On DDL integration complete we've get rid this mechanism. Nits: =E2=80=9Ccompletion=E2=80=9D, =E2=80=9Cgot rid of=E2=80=9D. > Now it should be returned. Well, actually your approach is slightly different: explain that DDL (to be more precise - triggers and checks creation) relies on parse_only flag in parser. Hence, you can check it and throw an error during parsing. >=20 > Closes #3653. > --- > src/box/space_def.c | 3 ++- > src/box/sql/parse.y | 6 +++++- > src/box/sql/tokenize.c | 8 ++++---- > test/sql-tap/check.test.lua | 4 ++-- > test/sql/checks.result | 45 = ++++++++++++++++++++++++++++++++++++++++++++- > test/sql/checks.test.lua | 18 +++++++++++++++++- > 6 files changed, 74 insertions(+), 10 deletions(-) >=20 > diff --git a/src/box/space_def.c b/src/box/space_def.c > index f5ca0b5..542289e 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > @@ -338,7 +338,8 @@ checks_array_decode(const char **str, uint32_t = len, char *opt, uint32_t errcode, > box_error_t *err =3D box_error_last(); > if (box_error_code(err) !=3D ENOMEM) { > snprintf(errmsg, TT_STATIC_BUF_LEN, > - "invalid expression = specified"); > + "invalid expression specified = (%s)", > + box_error_message(err)); > diag_set(ClientError, errcode, field_no, > errmsg); > } > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index d8532d3..60cf3f3 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -881,7 +881,11 @@ term(A) ::=3D INTEGER(X). { > } > expr(A) ::=3D VARIABLE(X). { > Token t =3D X; > - if( !(X.z[0]=3D=3D'#' && sqlite3Isdigit(X.z[1])) ){ > + if (pParse->parse_only) { > + spanSet(&A, &t, &t); > + sqlite3ErrorMsg(pParse, "bindings are not allowed in DDL"); > + A.pExpr =3D NULL; > + } else if (!(X.z[0]=3D=3D'#' && sqlite3Isdigit(X.z[1]))) { > u32 n =3D X.n; > spanExpr(&A, pParse, TK_VARIABLE, X); > if (A.pExpr->u.zToken[0] =3D=3D '?' && n > 1) > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index ec06456..4eebfe5 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -561,10 +561,10 @@ sql_expr_compile(sqlite3 *db, const char *expr, = int expr_len) > } > sprintf(stmt, "%s%.*s", outer, expr_len, expr); >=20 > - char *unused; > - if (sqlite3RunParser(&parser, stmt, &unused) !=3D SQLITE_OK || > + char *sql_error =3D NULL; > + if (sqlite3RunParser(&parser, stmt, &sql_error) !=3D SQLITE_OK = || > parser.parsed_ast_type !=3D AST_TYPE_EXPR) { > - diag_set(ClientError, ER_SQL_EXECUTE, stmt); > + diag_set(ClientError, ER_SQL, sql_error); > } else { > expression =3D parser.parsed_ast.expr; > parser.parsed_ast.expr =3D NULL; > @@ -602,7 +602,7 @@ sql_trigger_compile(struct sqlite3 *db, const char = *sql) > struct Parse parser; > sql_parser_create(&parser, db); > parser.parse_only =3D true; > - char *sql_error; > + char *sql_error =3D NULL; > struct sql_trigger *trigger =3D NULL; > if (sqlite3RunParser(&parser, sql, &sql_error) !=3D SQLITE_OK || > parser.parsed_ast_type !=3D AST_TYPE_TRIGGER) { > diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > index ff36552..f03ac7b 100755 > --- a/test/sql-tap/check.test.lua > +++ b/test/sql-tap/check.test.lua > @@ -555,7 +555,7 @@ test:do_catchsql_test( > ); > ]], { > -- > - 1, "Failed to create space 'T5': SQL error: parameters = prohibited in CHECK constraints" > + 1, "Wrong space options (field 5): invalid expression = specified (SQL error: bindings are not allowed in DDL)" > -- > }) Could we keep previous error message? It looks satisfactory actually. The same for triggers: could we use message like =E2=80=9CFailed to create trigger =E2=80=98=E2=80=A6=E2=80=99: = parameters prohibited in trigger definition=E2=80=9D? Or present your persuasive arguments :) > diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua > index fb95809..3506d5c 100644 > --- a/test/sql/checks.test.lua > +++ b/test/sql/checks.test.lua > @@ -43,11 +43,27 @@ format =3D {{name =3D 'X', type =3D 'unsigned'}} > t =3D {513, 1, 'test', 'memtx', 0, opts, format} > s =3D box.space._space:insert(t) >=20 > - > -- > -- gh-3611: Segfault on table creation with check referencing this = table > -- > box.sql.execute("CREATE TABLE w2 (s1 INT PRIMARY KEY, CHECK ((SELECT = COUNT(*) FROM w2) =3D 0));") > box.sql.execute("DROP TABLE w2;") >=20 > +-- > +-- gh-3653: Dissallow bindings for DDL > +-- > +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT);") > +space_id =3D box.space.T1.id > +box.sql.execute("CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a =3D = ? BEGIN SELECT 1; END;") > +tuple =3D {"TR1", space_id, {sql =3D [[CREATE TRIGGER tr1 AFTER = INSERT ON t1 WHEN new.a =3D ? BEGIN SELECT 1; END;]]}} This test should be moved to test/sql/triggers.test.lua (Since this test is about checks only).