[tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments and review fixes below.

On 10/12/2018 00:30, Nikita Pettik wrote:
> At the last stage of trigger creation, trigger's create statement
> ("CREATE TRIGGER ...") is encoded to msgpack. Since only this string is
> only member of a map to be encoded, is makes no sense to call whole
> sql_encode_table_opts() function, which in turn processes table's
> checks, opts for VIEW etc.
> 
> Needed for #2647
> ---
>   src/box/sql/trigger.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index c38f9cd9d..c7d22b9d0 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -212,19 +212,17 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
>   		parse->nMem += 3;
>   		int record = ++parse->nMem;
>   
> -		uint32_t opts_buff_sz = 0;
> -		char *data = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
> -						   &opts_buff_sz);
> -		sqlite3DbFree(db, sql_str);
> -		if (data == NULL) {
> -			parse->nErr++;
> -			parse->rc = SQL_TARANTOOL_ERROR;
> -			goto cleanup;
> -		}
> -		char *opts_buff = sqlite3DbMallocRaw(db, opts_buff_sz);
> +		uint32_t opts_buff_sz = mp_sizeof_map(1) +
> +					mp_sizeof_str(strlen("sql")) +
> +					mp_sizeof_str(strlen(sql_str));
> +		char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
>   		if (opts_buff == NULL)
>   			goto cleanup;
> -		memcpy(opts_buff, data, opts_buff_sz);
> +
> +		char *data = mp_encode_map(opts_buff, 1);
> +		data = mp_encode_str(data, "sql", strlen("sql"));
> +		data = mp_encode_str(data, sql_str, strlen(sql_str));

1. strlen(sql_str) is called twice. Foreboding your claim that
it will be optimized to 1 call - no, it won't. I disassembled this
function and saw double strlen(sql_str). Only strlen("sql") is
optimized to constant 3.

2. After you've deleted this invocation of sql_encode_table_opts(),
struct Table *table in its second parameter became not NULL always.
So I simplified this function a bit.

> +		sqlite3DbFree(db, sql_str);
>   
>   		trigger_name = sqlite3DbStrDup(db, trigger_name);
>   		if (trigger_name == NULL) {
> 

My review fixes here and on the branch:

==================================================================

commit c287163409a6a542273d07cb0602834dfa66df88
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date:   Mon Dec 10 16:12:23 2018 +0300

     Review fixes

diff --git a/src/box/sql.c b/src/box/sql.c
index 7b41c9926..f43387f18 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1199,16 +1199,13 @@ sql_encode_table_opts(struct region *region, struct Table *table,
  	bool is_error = false;
  	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
  		      set_encode_error, &is_error);
-	bool is_view = false;
  	int checks_cnt = 0;
  	struct ExprList_item *a;
-	if (table != NULL) {
-		is_view = table->def->opts.is_view;
-		struct ExprList *checks = table->def->opts.checks;
-		if (checks != NULL) {
-			checks_cnt = checks->nExpr;
-			a = checks->a;
-		}
+	bool is_view = table->def->opts.is_view;
+	struct ExprList *checks = table->def->opts.checks;
+	if (checks != NULL) {
+		checks_cnt = checks->nExpr;
+		a = checks->a;
  	}
  	mpstream_encode_map(&stream, 1 + is_view + (checks_cnt > 0));
  
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index c7d22b9d0..418dc9147 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -211,17 +211,19 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
  		int first_col = parse->nMem + 1;
  		parse->nMem += 3;
  		int record = ++parse->nMem;
+		int sql_str_len = strlen(sql_str);
+		int sql_len = strlen("sql");
  
  		uint32_t opts_buff_sz = mp_sizeof_map(1) +
-					mp_sizeof_str(strlen("sql")) +
-					mp_sizeof_str(strlen(sql_str));
+					mp_sizeof_str(sql_len) +
+					mp_sizeof_str(sql_str_len);
  		char *opts_buff = (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
  		if (opts_buff == NULL)
  			goto cleanup;
  
  		char *data = mp_encode_map(opts_buff, 1);
-		data = mp_encode_str(data, "sql", strlen("sql"));
-		data = mp_encode_str(data, sql_str, strlen(sql_str));
+		data = mp_encode_str(data, "sql", sql_len);
+		data = mp_encode_str(data, sql_str, sql_str_len);
  		sqlite3DbFree(db, sql_str);
  
  		trigger_name = sqlite3DbStrDup(db, trigger_name);