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 3C983308DE for ; Fri, 30 Nov 2018 12:42:21 -0500 (EST) 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 AsFmgVnuxIGZ for ; Fri, 30 Nov 2018 12:42:21 -0500 (EST) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 EAE72308D7 for ; Fri, 30 Nov 2018 12:42:20 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: fix parser.parse_only mode for triggers References: <0943b4db119f16a30aad8198943568521508dfdb.1543507402.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <45caa0d1-606e-66b2-9971-d0156cd087ad@tarantool.org> Date: Fri, 30 Nov 2018 20:42:13 +0300 MIME-Version: 1.0 In-Reply-To: <0943b4db119f16a30aad8198943568521508dfdb.1543507402.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! 1. I do not see branch and issue links. On 29/11/2018 19:03, Kirill Shcherbatov wrote: > The sql_trigger_compile routine had a Vdbe leak as parser doesn't > releases VM(it is not required in typical scenario). As the > parse_only flag had not worked correctly for sql triggers > sql_trigger_compile have had a memory leak. > > Closes #3838 > --- > src/box/sql.c | 1 + > src/box/sql/tokenize.c | 3 +++ > src/box/sql/trigger.c | 2 ++ > 3 files changed, 6 insertions(+) > > diff --git a/src/box/sql.c b/src/box/sql.c > index 7b41c9926..4ac55485f 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1535,6 +1535,7 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, > diag_set(ClientError, ER_SQL, parser.zErrMsg); > rc = -1; > } > + assert(parser.pVdbe == NULL); > sql_parser_destroy(&parser); > return rc; > } > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 4eebfe527..38a90eaba 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -570,6 +570,7 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len) > parser.parsed_ast.expr = NULL; > } > end: > + assert(parser.pVdbe == NULL); > sql_parser_destroy(&parser); > return expression; > } > @@ -592,6 +593,7 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt) > parser.parsed_ast.select = NULL; > } > > + assert(parser.pVdbe == NULL); > sql_parser_destroy(&parser); > return select; > } > @@ -613,6 +615,7 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql) > parser.parsed_ast.trigger = NULL; > } > > + assert(parser.pVdbe == NULL); > sql_parser_destroy(&parser); > return trigger; > } > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index c38f9cd9d..593504d38 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -249,6 +249,8 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list, > parse->parsed_ast.trigger = trigger; > parse->parsed_ast_type = AST_TYPE_TRIGGER; > trigger = NULL; > + sqlite3VdbeDelete(parse->pVdbe); > + parse->pVdbe = NULL; 2. This Vdbe objects oscillation makes no sense. You shall not even create Vdbe if parse_only is true. > } > > cleanup: > Consider my review fixes below and on the branch in a separate commit: =================================================================== commit 121fa344bf41cd478f42ff0561573efb214e6066 Author: Vladislav Shpilevoy Date: Fri Nov 30 20:39:04 2018 +0300 Review fixes diff --git a/src/box/sql.c b/src/box/sql.c index 4ac55485f..7b41c9926 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1535,7 +1535,6 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list, diag_set(ClientError, ER_SQL, parser.zErrMsg); rc = -1; } - assert(parser.pVdbe == NULL); sql_parser_destroy(&parser); return rc; } diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c index a4b65ebe7..824578e45 100644 --- a/src/box/sql/prepare.c +++ b/src/box/sql/prepare.c @@ -282,6 +282,7 @@ void sql_parser_destroy(Parse *parser) { assert(parser != NULL); + assert(!parser->parse_only || parser->pVdbe == NULL); sqlite3 *db = parser->db; sqlite3DbFree(db, parser->aLabel); sql_expr_list_delete(db, parser->pConstExpr); diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 38a90eaba..4eebfe527 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -570,7 +570,6 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len) parser.parsed_ast.expr = NULL; } end: - assert(parser.pVdbe == NULL); sql_parser_destroy(&parser); return expression; } @@ -593,7 +592,6 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt) parser.parsed_ast.select = NULL; } - assert(parser.pVdbe == NULL); sql_parser_destroy(&parser); return select; } @@ -615,7 +613,6 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql) parser.parsed_ast.trigger = NULL; } - assert(parser.pVdbe == NULL); sql_parser_destroy(&parser); return trigger; } diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 593504d38..482f40926 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -73,10 +73,6 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm, /* The name of the Trigger. */ char *trigger_name = NULL; - struct Vdbe *v = sqlite3GetVdbe(parse); - if (v != NULL) - sqlite3VdbeCountChanges(v); - /* pName->z might be NULL, but not pName itself. */ assert(name != NULL); assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); @@ -84,13 +80,6 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm, if (table == NULL || db->mallocFailed) goto trigger_cleanup; - - /* - * Ensure the table name matches database name and that - * the table exists. - */ - if (db->mallocFailed) - goto trigger_cleanup; assert(table->nSrc == 1); trigger_name = sqlite3NameFromToken(db, name); @@ -111,6 +100,9 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm, } if (!parse->parse_only) { + struct Vdbe *v = sqlite3GetVdbe(parse); + if (v != NULL) + sqlite3VdbeCountChanges(v); const char *error_msg = tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), trigger_name); @@ -249,8 +241,6 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list, parse->parsed_ast.trigger = trigger; parse->parsed_ast_type = AST_TYPE_TRIGGER; trigger = NULL; - sqlite3VdbeDelete(parse->pVdbe); - parse->pVdbe = NULL; } cleanup: diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index f2faad862..8dff4faf5 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -50,6 +50,7 @@ Vdbe * sqlite3VdbeCreate(Parse * pParse) { + assert(! pParse->parse_only); sqlite3 *db = pParse->db; Vdbe *p; p = sqlite3DbMallocRawNN(db, sizeof(Vdbe));