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 85A9921A69 for ; Wed, 12 Dec 2018 07:36:22 -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 IQKL_SXVepIp for ; Wed, 12 Dec 2018 07:36:22 -0500 (EST) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 3549A2151D for ; Wed, 12 Dec 2018 07:36:22 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 2/6] sql: don't update SQL string during renaming References: <02252f24-c2cb-d9f3-c327-48015855c1dc@tarantool.org> <4440A966-E81F-4F72-9091-C4160C033B7D@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 12 Dec 2018 15:36:18 +0300 MIME-Version: 1.0 In-Reply-To: <4440A966-E81F-4F72-9091-C4160C033B7D@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: "n.pettik" , tarantool-patches@freelists.org 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(-) >> >> You forgot to remove some code and comments. My >> review fixes here and on the branch. > > Thx, applied. > 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. After these changes are applied, the patchset is LGTM. ==================================================================== commit 1d280e440cb3049bd45bfffa4243d8d1fb5d1c3d Author: Vladislav Shpilevoy Date: Wed Dec 12 15:27:12 2018 +0300 Review fix 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 != 0); assert(new_name != NULL); + int name_len = strlen(new_name); struct region *region = &fiber()->gc; - size_t used = region_used(region); - struct mpstream stream; - bool is_error = 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 = stream.pos - stream.buf; - mpstream_encode_array(&stream, 1); - mpstream_encode_array(&stream, 3); - mpstream_encode_str(&stream, "="); - 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 = region_used(region) - used; - char *raw = region_join(region, sz); + /* 32 + name_len is enough to encode one update op. */ + size_t size = 32 + name_len; + char *raw = (char *) region_alloc(region, size); if (raw == 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) != 0) + /* Encode key. */ + char *pos = mp_encode_array(raw, 1); + pos = mp_encode_uint(pos, space_id); + + /* Encode op and new name. */ + char *ops = pos; + pos = mp_encode_array(pos, 1); + pos = mp_encode_array(pos, 3); + pos = mp_encode_str(pos, "=", 1); + pos = mp_encode_uint(pos, BOX_SPACE_FIELD_NAME); + pos = mp_encode_str(pos, new_name, name_len); + if (box_update(BOX_SPACE_ID, 0, raw, ops, ops, pos, 0, NULL) != 0) return SQL_TARANTOOL_ERROR; return 0; }