[tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation
n.pettik
korablev at tarantool.org
Tue Dec 11 21:29:16 MSK 2018
> 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>
More information about the Tarantool-patches
mailing list