From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 40751210C0 for ; Tue, 11 Dec 2018 13:29:23 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dDRz-wUQTIlS for ; Tue, 11 Dec 2018 13:29:23 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 3B37120C3D for ; Tue, 11 Dec 2018 13:29:20 -0500 (EST) From: "n.pettik" Message-Id: <6C1B6665-AE92-4ADA-BA1F-B1A3973D544C@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_7FC7F4FA-88D6-4865-9B51-6B95BB831E2A" Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 1/6] sql: avoid calling sql_encode_table_opts() during trigger creation Date: Tue, 11 Dec 2018 21:29:16 +0300 In-Reply-To: <40776123-c105-aaae-8ed7-e2ad70c232e3@tarantool.org> References: <3c4a6fd745c0a651a164cdc155404e3555078a56.1544387419.git.korablev@tarantool.org> <40776123-c105-aaae-8ed7-e2ad70c232e3@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy --Apple-Mail=_7FC7F4FA-88D6-4865-9B51-6B95BB831E2A Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 10 Dec 2018, at 17:17, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! See 2 comments and review fixes below. >=20 > 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 +=3D 3; >> int record =3D ++parse->nMem; >> - uint32_t opts_buff_sz =3D 0; >> - char *data =3D sql_encode_table_opts(&fiber()->gc, NULL, = sql_str, >> - &opts_buff_sz); >> - sqlite3DbFree(db, sql_str); >> - if (data =3D=3D NULL) { >> - parse->nErr++; >> - parse->rc =3D SQL_TARANTOOL_ERROR; >> - goto cleanup; >> - } >> - char *opts_buff =3D sqlite3DbMallocRaw(db, = opts_buff_sz); >> + uint32_t opts_buff_sz =3D mp_sizeof_map(1) + >> + mp_sizeof_str(strlen("sql")) + >> + mp_sizeof_str(strlen(sql_str)); >> + char *opts_buff =3D (char *) sqlite3DbMallocRaw(db, = opts_buff_sz); >> if (opts_buff =3D=3D NULL) >> goto cleanup; >> - memcpy(opts_buff, data, opts_buff_sz); >> + >> + char *data =3D mp_encode_map(opts_buff, 1); >> + data =3D mp_encode_str(data, "sql", strlen("sql")); >> + data =3D mp_encode_str(data, sql_str, strlen(sql_str)); >=20 > 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=E2=80=99m too lazy to check it, so let it be. >=20 > 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. >=20 >> + sqlite3DbFree(db, sql_str); >> trigger_name =3D sqlite3DbStrDup(db, trigger_name); >> if (trigger_name =3D=3D NULL) { >=20 > My review fixes here and on the branch: Ok, applied. --Apple-Mail=_7FC7F4FA-88D6-4865-9B51-6B95BB831E2A Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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 +=3D 3;
  int = record =3D ++parse->nMem;
 - uint32_t = opts_buff_sz =3D 0;
- char *data =3D = sql_encode_table_opts(&fiber()->gc, NULL, sql_str,
- = = = = = =    &opts_buff_sz= );
- sqlite3DbFree(db, sql_str);
- if (data = =3D=3D NULL) {
- parse->nErr++;
- = = = parse->rc =3D SQL_TARANTOOL_ERROR;
- goto = cleanup;
- }
- char = *opts_buff =3D sqlite3DbMallocRaw(db, opts_buff_sz);
+ uint32_t = opts_buff_sz =3D mp_sizeof_map(1) +
+ = mp_sizeof_str(strlen("sql")) +
+ = mp_sizeof_str(strlen(sql_str));
+ char = *opts_buff =3D (char *) sqlite3DbMallocRaw(db, opts_buff_sz);
  if (opts_buff =3D=3D NULL)
  goto = cleanup;
- memcpy(opts_buff, data, = opts_buff_sz);
+
+ char = *data =3D mp_encode_map(opts_buff, 1);
+ data =3D = mp_encode_str(data, "sql", strlen("sql"));
+ data =3D = 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=E2=80=99m 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 =3D sqlite3DbStrDup(db, trigger_name);
  if (trigger_name =3D=3D NULL) {

My review fixes here and on the branch:

Ok, = applied.

= --Apple-Mail=_7FC7F4FA-88D6-4865-9B51-6B95BB831E2A--