[tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL

["n.pettik" <korablev@tarantool.org>]

Previous two patches looks OK to me.

> Bindings could not be used in stored ACTs because they allocate

Nit: “AST”.

> memory registers and makes

Nit: “make” (or better “process").

> assignments on parse sequentially.

Nit: “during parsing”.

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’t come up with example how they can be used there at all).
b) For triggers currently we don’t have proper mechanism, which
would allow to use bindings. Original SQLite also lacks it.
To reuse the same trigger’s 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. 

> 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: “completion”, “got rid of”.

> 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.

> 
> 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(-)
> 
> 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 = box_error_last();
> 			if (box_error_code(err) != 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) ::= INTEGER(X). {
> }
> expr(A) ::= VARIABLE(X).     {
>   Token t = X;
> -  if( !(X.z[0]=='#' && sqlite3Isdigit(X.z[1])) ){
> +  if (pParse->parse_only) {
> +    spanSet(&A, &t, &t);
> +    sqlite3ErrorMsg(pParse, "bindings are not allowed in DDL");
> +    A.pExpr = NULL;
> +  } else if (!(X.z[0]=='#' && sqlite3Isdigit(X.z[1]))) {
>     u32 n = X.n;
>     spanExpr(&A, pParse, TK_VARIABLE, X);
>     if (A.pExpr->u.zToken[0] == '?' && 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);
> 
> -	char *unused;
> -	if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK ||
> +	char *sql_error = NULL;
> +	if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK ||
> 	    parser.parsed_ast_type != AST_TYPE_EXPR) {
> -		diag_set(ClientError, ER_SQL_EXECUTE, stmt);
> +		diag_set(ClientError, ER_SQL, sql_error);
> 	} else {
> 		expression = parser.parsed_ast.expr;
> 		parser.parsed_ast.expr = 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 = true;
> -	char *sql_error;
> +	char *sql_error = NULL;
> 	struct sql_trigger *trigger = NULL;
> 	if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK ||
> 	    parser.parsed_ast_type != 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(
>         );
>     ]], {
>         -- <check-5.1>
> -        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)"
>         -- </check-5.1>
>     })

Could we keep previous error message? It looks satisfactory actually.
The same for triggers: could we use message like
“Failed to create trigger ‘…’: parameters prohibited in trigger definition”?
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 = {{name = 'X', type = 'unsigned'}}
> t = {513, 1, 'test', 'memtx', 0, opts, format}
> s = box.space._space:insert(t)
> 
> -
> --
> -- 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) = 0));")
> box.sql.execute("DROP TABLE w2;")
> 
> +--
> +-- gh-3653: Dissallow bindings for DDL
> +--
> +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT);")
> +space_id = box.space.T1.id
> +box.sql.execute("CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a = ? BEGIN SELECT 1; END;")
> +tuple = {"TR1", space_id, {sql = [[CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a = ? BEGIN SELECT 1; END;]]}}

This test should be moved to test/sql/triggers.test.lua
(Since this test is about checks only).