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

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: