[tarantool-patches] Re: [PATCH v2 1/1] sql: fix parser.parse_only mode for triggers
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Nov 30 20:42:13 MSK 2018
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 <v.shpilevoy at tarantool.org>
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));
More information about the Tarantool-patches
mailing list