[tarantool-patches] Re: [PATCH v2 1/1] sql: fix parser.parse_only mode for triggers

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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 <v.shpilevoy@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));