* [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL @ 2018-08-31 15:45 Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free Kirill Shcherbatov ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Kirill Shcherbatov @ 2018-08-31 15:45 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free 2018-08-31 15:45 [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL Kirill Shcherbatov @ 2018-08-31 15:45 ` Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 2/3] sql: fix sql_*_compile functions leak on error Kirill Shcherbatov ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Kirill Shcherbatov @ 2018-08-31 15:45 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v1 2/3] sql: fix sql_*_compile functions leak on error 2018-08-31 15:45 [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free Kirill Shcherbatov @ 2018-08-31 15:45 ` Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 3/3] sql: dissallow bindings for DDL Kirill Shcherbatov 2018-09-13 10:12 ` [tarantool-patches] Re: [PATCH v1 0/3] " Kirill Yukhin 3 siblings, 0 replies; 11+ messages in thread From: Kirill Shcherbatov @ 2018-08-31 15:45 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-08-31 15:45 [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 2/3] sql: fix sql_*_compile functions leak on error Kirill Shcherbatov @ 2018-08-31 15:45 ` Kirill Shcherbatov 2018-09-04 11:00 ` [tarantool-patches] " n.pettik 2018-09-13 10:12 ` [tarantool-patches] Re: [PATCH v1 0/3] " Kirill Yukhin 3 siblings, 1 reply; 11+ messages in thread From: Kirill Shcherbatov @ 2018-08-31 15:45 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev, 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 3/3] sql: dissallow bindings for DDL Kirill Shcherbatov @ 2018-09-04 11:00 ` n.pettik 2018-09-06 13:04 ` Kirill Shcherbatov 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-09-04 11:00 UTC (permalink / raw) To: tarantool-patches; +Cc: 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”. 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). ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-09-04 11:00 ` [tarantool-patches] " n.pettik @ 2018-09-06 13:04 ` Kirill Shcherbatov 2018-09-10 21:52 ` n.pettik 0 siblings, 1 reply; 11+ messages in thread From: Kirill Shcherbatov @ 2018-09-06 13:04 UTC (permalink / raw) To: tarantool-patches, Nikita 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”. > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-09-06 13:04 ` Kirill Shcherbatov @ 2018-09-10 21:52 ` n.pettik 2018-09-11 7:21 ` Kirill Shcherbatov 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-09-10 21:52 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov > 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). ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-09-10 21:52 ` n.pettik @ 2018-09-11 7:21 ` Kirill Shcherbatov 2018-09-11 23:03 ` n.pettik 0 siblings, 1 reply; 11+ messages in thread From: Kirill Shcherbatov @ 2018-09-11 7:21 UTC (permalink / raw) To: tarantool-patches, Nikita 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-09-11 7:21 ` Kirill Shcherbatov @ 2018-09-11 23:03 ` n.pettik 2018-09-13 6:13 ` Kirill Shcherbatov 0 siblings, 1 reply; 11+ messages in thread From: n.pettik @ 2018-09-11 23:03 UTC (permalink / raw) To: tarantool-patches; +Cc: 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. 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] sql: dissallow bindings for DDL 2018-09-11 23:03 ` n.pettik @ 2018-09-13 6:13 ` Kirill Shcherbatov 0 siblings, 0 replies; 11+ messages in thread From: Kirill Shcherbatov @ 2018-09-13 6:13 UTC (permalink / raw) To: tarantool-patches, Kirill Yukhin > - > - > > 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v1 0/3] sql: dissallow bindings for DDL 2018-08-31 15:45 [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL Kirill Shcherbatov ` (2 preceding siblings ...) 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 3/3] sql: dissallow bindings for DDL Kirill Shcherbatov @ 2018-09-13 10:12 ` Kirill Yukhin 3 siblings, 0 replies; 11+ messages in thread From: Kirill Yukhin @ 2018-09-13 10:12 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev, Kirill Shcherbatov 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-09-13 10:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-31 15:45 [tarantool-patches] [PATCH v1 0/3] sql: dissallow bindings for DDL Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 1/3] sql: fix sql_check_list_item_init double free Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 2/3] sql: fix sql_*_compile functions leak on error Kirill Shcherbatov 2018-08-31 15:45 ` [tarantool-patches] [PATCH v1 3/3] sql: dissallow bindings for DDL Kirill Shcherbatov 2018-09-04 11:00 ` [tarantool-patches] " n.pettik 2018-09-06 13:04 ` Kirill Shcherbatov 2018-09-10 21:52 ` n.pettik 2018-09-11 7:21 ` Kirill Shcherbatov 2018-09-11 23:03 ` n.pettik 2018-09-13 6:13 ` Kirill Shcherbatov 2018-09-13 10:12 ` [tarantool-patches] Re: [PATCH v1 0/3] " Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox