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 01C9C2212B for ; Thu, 13 Dec 2018 07:42:12 -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 SV8xItokZGe6 for ; Thu, 13 Dec 2018 07:42:11 -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 3972722129 for ; Thu, 13 Dec 2018 07:42:11 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming From: "n.pettik" In-Reply-To: Date: Thu, 13 Dec 2018 15:42:07 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <937C4C0D-DCB6-4C76-BEB8-804331C25F0C@tarantool.org> References: <02252f24-c2cb-d9f3-c327-48015855c1dc@tarantool.org> <4440A966-E81F-4F72-9091-C4160C033B7D@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 > On 12 Dec 2018, at 15:36, Vladislav Shpilevoy = wrote: > On 11/12/2018 21:29, n.pettik wrote: >>> On 10 Dec 2018, at 17:16, Vladislav Shpilevoy = wrote: >>> Thanks for the patch! >>> On 10/12/2018 00:30, Nikita Pettik wrote: >>>> Since SQL string containing "CREATE TABLE ..." statement is not = used >>>> anymore for ordinary tables/space, it makes no sense to modify it = during >>>> renaming. Hence, now rename routine needs only to update name in = _space, >>>> so it can be done using simple update operation. >>>> Moreover, now we are able to rename spaces created from Lua-land. >>>> Part of #2647 >>>> --- >>>> src/box/sql.c | 187 = ++++++++------------------------------------ >>>> src/box/sql/tarantoolInt.h | 3 +- >>>> src/box/sql/vdbe.c | 4 +- >>>> test/sql-tap/alter.test.lua | 32 +++++++- >>>> 4 files changed, 63 insertions(+), 163 deletions(-) >>>=20 >>> You forgot to remove some code and comments. My >>> review fixes here and on the branch. >> Thx, applied. >=20 > Sorry, I've found another space for optimizations. See my > new review fixes below and on the branch. This time I've > removed mpstream from sql_rename_table(). It'd used mpstream > only because of mpstream_encode_index_opts, which was removed > from there by your commit. >=20 > After these changes are applied, the patchset is LGTM. >=20 Ok, I=E2=80=99ve applied your fixes. Nevertheless, I don=E2=80=99t think = that such nano =E2=80=9Coptimizations=E2=80=9D worth it IMHO. At least due to the = fact that rename is assumed to be called rarely, so it is not hot path. > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > commit 1d280e440cb3049bd45bfffa4243d8d1fb5d1c3d > Author: Vladislav Shpilevoy > Date: Wed Dec 12 15:27:12 2018 +0300 >=20 > Review fix >=20 > diff --git a/src/box/sql.c b/src/box/sql.c > index 0a10f4d8c..18e8f2507 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -709,37 +709,27 @@ sql_rename_table(uint32_t space_id, const char = *new_name) > { > assert(space_id !=3D 0); > assert(new_name !=3D NULL); > + int name_len =3D strlen(new_name); > struct region *region =3D &fiber()->gc; > - size_t used =3D region_used(region); > - struct mpstream stream; > - bool is_error =3D false; > - mpstream_init(&stream, region, region_reserve_cb, = region_alloc_cb, > - set_encode_error, &is_error); > - /* Encode key. */ > - mpstream_encode_array(&stream, 1); > - mpstream_encode_uint(&stream, space_id); > - > - /* Encode op and new name. */ > - uint32_t op_offset =3D stream.pos - stream.buf; > - mpstream_encode_array(&stream, 1); > - mpstream_encode_array(&stream, 3); > - mpstream_encode_str(&stream, "=3D"); > - mpstream_encode_uint(&stream, BOX_SPACE_FIELD_NAME); > - mpstream_encode_str(&stream, new_name); > - mpstream_flush(&stream); > - if (is_error) { > - diag_set(OutOfMemory, stream.pos - stream.buf, > - "mpstream_flush", "stream"); > - return SQL_TARANTOOL_ERROR; > - } > - size_t sz =3D region_used(region) - used; > - char *raw =3D region_join(region, sz); > + /* 32 + name_len is enough to encode one update op. */ > + size_t size =3D 32 + name_len; > + char *raw =3D (char *) region_alloc(region, size); > if (raw =3D=3D NULL) { > - diag_set(OutOfMemory, sz, "region_join", "raw"); > + diag_set(OutOfMemory, size, "region_alloc", "raw"); > return SQL_TARANTOOL_ERROR; > } > - if (box_update(BOX_SPACE_ID, 0, raw, raw + op_offset, raw + = op_offset, > - raw + sz, 0, NULL) !=3D 0) > + /* Encode key. */ > + char *pos =3D mp_encode_array(raw, 1); > + pos =3D mp_encode_uint(pos, space_id); > + > + /* Encode op and new name. */ > + char *ops =3D pos; > + pos =3D mp_encode_array(pos, 1); > + pos =3D mp_encode_array(pos, 3); > + pos =3D mp_encode_str(pos, "=3D", 1); > + pos =3D mp_encode_uint(pos, BOX_SPACE_FIELD_NAME); > + pos =3D mp_encode_str(pos, new_name, name_len); > + if (box_update(BOX_SPACE_ID, 0, raw, ops, ops, pos, 0, NULL) !=3D = 0) > return SQL_TARANTOOL_ERROR; > return 0; > }