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

[n.pettik]

> On 10 Dec 2018, at 17:17, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> 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.

I’m too lazy to check it, so let it be.

> 
> 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:

Ok, applied.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181211/8e3124d9/attachment.html>