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

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

[Kirill Shcherbatov]

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3653-no-bindings-on-ddl
Issue: https://github.com/tarantool/tarantool/issues/3653

Bindings could not be used in stored ACTs because they allocate
memory registers and makes assignments on parse sequentially.
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.
Now it should be returned.
Also fixed memory leak on error in sql_*_compile functions and
double free with sql_check_list_item_init.

Kirill Shcherbatov (3):
  sql: fix sql_check_list_item_init double free
  sql: fix sql_*_compile functions leak on error
  sql: dissallow bindings for DDL

 src/box/space_def.c         |  3 ++-
 src/box/sql.c               |  5 ++---
 src/box/sql/parse.y         |  6 +++++-
 src/box/sql/prepare.c       |  1 -
 src/box/sql/tokenize.c      |  9 ++++-----
 test/sql-tap/check.test.lua |  4 ++--
 test/sql/checks.result      | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 test/sql/checks.test.lua    | 18 +++++++++++++++++-
 8 files changed, 76 insertions(+), 15 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free

[Kirill Shcherbatov]

---
 src/box/sql.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index b158c50..d3f50c5 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1711,10 +1711,9 @@ sql_check_list_item_init(struct ExprList *expr_list, int column,
 	}
 	if (expr_str != NULL) {
 		item->pExpr = sql_expr_compile(db, expr_str, expr_str_len);
-		if (item->pExpr == NULL) {
-			sqlite3DbFree(db, item->zName);
+		/* The item->zName would be released later. */
+		if (item->pExpr == NULL)
 			return -1;
-		}
 	}
 	return 0;
 }
-- 
2.7.4

[tarantool-patches] [PATCH v1 2/3] sql: fix sql_*_compile functions leak on error

[Kirill Shcherbatov]

---
 src/box/sql/prepare.c  | 1 -
 src/box/sql/tokenize.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e8b8e94..a6af683 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -279,7 +279,6 @@ sqlite3Prepare(sqlite3 * db,	/* Database handle. */
 
 	if (zErrMsg) {
 		sqlite3ErrorWithMsg(db, rc, "%s", zErrMsg);
-		sqlite3DbFree(db, zErrMsg);
 	} else {
 		sqlite3Error(db, rc);
 	}
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index ce9ed84..ec06456 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -523,7 +523,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 	if (pParse->zErrMsg) {
 		*pzErrMsg = pParse->zErrMsg;
 		sqlite3_log(pParse->rc, "%s", *pzErrMsg);
-		pParse->zErrMsg = 0;
 		nErr++;
 	}
 	if (pParse->pVdbe != NULL && pParse->nErr > 0) {
-- 
2.7.4

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

[Kirill Shcherbatov]

Bindings could not be used in stored ACTs because they allocate
memory registers and makes assignments on parse sequentially.
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.
Now it should be returned.

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>
     })
 
@@ -567,7 +567,7 @@ test:do_catchsql_test(
         );
     ]], {
         -- <check-5.2>
-        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.2>
     })
 
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 3084d89..a88e048 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -29,7 +29,8 @@ t = {513, 1, 'test', 'memtx', 0, opts, format}
 ...
 s = box.space._space:insert(t)
 ---
-- error: 'Wrong space options (field 5): invalid expression specified'
+- error: 'Wrong space options (field 5): invalid expression specified (SQL error:
+    near "<": syntax error)'
 ...
 opts = {checks = {{expr = 'X>5'}}}
 ---
@@ -116,6 +117,48 @@ box.sql.execute("DROP TABLE w2;")
 ---
 - error: 'no such 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;")
+---
+- error: 'SQL error: bindings are not allowed in DDL'
+...
+tuple = {"TR1", space_id, {sql = [[CREATE TRIGGER tr1 AFTER INSERT ON t1 WHEN new.a = ? BEGIN SELECT 1; END;]]}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: bindings are not allowed in DDL'
+...
+box.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("CREATE TABLE t5(x primary key, y,CHECK( x*y<? ));")
+---
+- error: 'Wrong space options (field 5): invalid expression specified (SQL error:
+    bindings are not allowed in DDL)'
+...
+opts = {checks = {{expr = '?>5', name = 'ONE'}}}
+---
+...
+format = {{name = 'X', type = 'unsigned'}}
+---
+...
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+---
+...
+s = box.space._space:insert(t)
+---
+- error: 'Wrong space options (field 5): invalid expression specified (SQL error:
+    bindings are not allowed in DDL)'
+...
 test_run:cmd("clear filter")
 ---
 - true
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;]]}}
+box.space._trigger:insert(tuple)
+box.sql.execute("DROP TABLE t1;")
+
+box.sql.execute("CREATE TABLE t5(x primary key, y,CHECK( x*y<? ));")
+
+opts = {checks = {{expr = '?>5', name = 'ONE'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
 test_run:cmd("clear filter")
-- 
2.7.4

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

[n.pettik]

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

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

[Kirill Shcherbatov]

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

    sql: dissallow bindings for DDL
    
    Bindings could not be used in stored ASTs because they allocate
    memory registers and process assignments during parsing(temporal register
    should be allocated for each variable in parse context, but it is temporal).
    Original sqlite3 did validations that AST to be persisted doesn't have
    auto-assigment Varibles on triggers and checks creation.
    On DDL integration completion we've got rid this mechanism.
    Now it should be returned.
    We use flag 'parse_only' which is set by parser on compiling AST to
    determine attempt to use bindings in DDL(triggers, defaults and checks
    creation) that is incorrect and raise an error.
    
    Closes #3653.

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

[n.pettik]

>    sql: dissallow bindings for DDL
> 
>    Bindings could not be used in stored ASTs because they allocate
>    memory registers and process assignments during parsing(temporal register
>    should be allocated for each variable in parse context, but it is temporal).
>    Original sqlite3 did validations that AST to be persisted doesn't have
>    auto-assigment Varibles on triggers and checks creation.
>    On DDL integration completion we've got rid this mechanism.
>    Now it should be returned.
>    We use flag 'parse_only' which is set by parser on compiling AST to
>    determine attempt to use bindings in DDL(triggers, defaults and checks
>    creation) that is incorrect and raise an error.
> 
>    Closes #3653.

Please, don’t *silently* skip my comments (yep, still they are minor but
should be taken into consideration since the rest of patch is OK):

‘''
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 :)
‘’’

I ‘member that we discussed smth about error messages but really
can’t recall exactly what. So, please, answer on this nit or fix it.

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

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

[Kirill Shcherbatov]

> ‘''
> 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 :)
> ‘’’
> 
> I ‘member that we discussed smth about error messages but really
> can’t recall exactly what. So, please, answer on this nit or fix it.
The previous error message was set elsewhere in other place, on resolve.
Now this is a part of parser that doesn't now, which AST does it compile. And this 
should be done uniformly.

> This test should be moved to test/sql/triggers.test.lua
> (Since this test is about checks only).
Ohm, okey. Separate tests per one ticket is a bigger evil for me.
As you wish. I've fixed it.

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

[n.pettik]

>> ‘''
>> 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 :)
>> ‘’’
>> 
>> I ‘member that we discussed smth about error messages but really
>> can’t recall exactly what. So, please, answer on this nit or fix it.
> The previous error message was set elsewhere in other place, on resolve.
> Now this is a part of parser that doesn't now, which AST does it compile. And this 
> should be done uniformly.
> 
>> This test should be moved to test/sql/triggers.test.lua
>> (Since this test is about checks only).
> Ohm, okey. Separate tests per one ticket is a bigger evil for me.
> As you wish. I've fixed it.

Please, don’t forget to attach diff.

diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index fb958092e..24d5ee725 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -42,12 +42,20 @@ opts = {checks = {{name = 123}}}
 format = {{name = 'X', type = 'unsigned'}}
 t = {513, 1, 'test', 'memtx', 0, opts, format}
 s = box.space._space:insert(t)
-
-

Noise diff.

 --
 -- 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 t5(x primary key, y,CHECK( x*y<? ));")
+
+opts = {checks = {{expr = '?>5', name = 'ONE'}}}
+format = {{name = 'X', type = 'unsigned'}}
+t = {513, 1, 'test', 'memtx', 0, opts, format}
+s = box.space._space:insert(t)
+
 test_run:cmd("clear filter”)

You forgot to drop table t5.

+++ b/test/sql/triggers.test.lua
@@ -155,3 +155,14 @@ box.sql.execute("UPDATE v SET s1 = s1 + 5;")
 box.sql.execute("SELECT * FROM t;")
 box.sql.execute("DROP VIEW v;")
 box.sql.execute("DROP TABLE t;")
+
+

One new line is redundant.

The rest *obviously* LGTM.

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

[Kirill Shcherbatov]

> -
> -
> 
> Noise diff.
Ok.

> You forgot to drop table t5.
It isn't required as this attempt to create t5 should fail that is a test scenario.

>  box.sql.execute("DROP TABLE t;")
> +
> +
> 
> One new line is redundant.
Ok.

> 
> The rest *obviously* LGTM.
Kirill, let's merge It.

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

[Kirill Yukhin]

Hello,
On 31 авг 18:45, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3653-no-bindings-on-ddl
> Issue: https://github.com/tarantool/tarantool/issues/3653

I've merged the patchset into 2.0 branch.

--
Regards, Kirill Yukhin