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 D9E4F21196 for ; Mon, 10 Dec 2018 09:18:55 -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 x3AWZaOlha_E for ; Mon, 10 Dec 2018 09:18:55 -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 9651120237 for ; Mon, 10 Dec 2018 09:18:55 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 5/6] sql: don't add string of 'CREATE INDEX ...' to index opts References: <988bd992af26db529d76e259d2498a7e04f958b1.1544387419.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 10 Dec 2018 17:18:53 +0300 MIME-Version: 1.0 In-Reply-To: <988bd992af26db529d76e259d2498a7e04f958b1.1544387419.git.korablev@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: tarantool-patches@freelists.org, Nikita Pettik Thanks for the patch! See 2 comments and review fixes below. On 10/12/2018 00:30, Nikita Pettik wrote: > Part of #2647 > --- > src/box/sql.c | 22 +++------------------- > src/box/sql/build.c | 35 +++-------------------------------- > 2 files changed, 6 insertions(+), 51 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index e79265823..45c539ec7 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1214,7 +1196,9 @@ sql_encode_index_opts(struct region *region, const struct index_opts *opts, > bool is_error = false; > mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb, > set_encode_error, &is_error); > - mpstream_encode_index_opts(&stream, opts); > + mpstream_encode_map(&stream, 1); > + mpstream_encode_str(&stream, "unique"); > + mpstream_encode_bool(&stream, opts->is_unique);; > mpstream_flush(&stream); 1. We do not need mpstream for this tiny piece of code. I refactored it to simple region_alloc. > if (is_error) { > diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush", > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index fd1666c6d..ab2a56420 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2546,6 +2527,7 @@ sql_create_index(struct Parse *parse, struct Token *token, > sqlite3VdbeAddOp1(vdbe, OP_Close, cursor); > vdbe_emit_create_index(parse, def, index->def, > def->id, index_id); > + sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE); 2. Are you sure that you should increment change counter here? As I know, it should be incremented once on a DDL, but this increment is under 'if (tbl_name != NULL)' which means that it is not a separate index, but part of CREATE TABLE, it is not? CREATE TABLE increments change counter on insertion into _space only. > sqlite3VdbeAddOp0(vdbe, OP_Expire); > } > My review fixes here and on the branch: ================================================================= commit 65020d36bb3a9c8058789c90ed95665bd847a732 Author: Vladislav Shpilevoy Date: Mon Dec 10 17:11:26 2018 +0300 Review fixes diff --git a/src/box/sql.c b/src/box/sql.c index d270529ae..dd7a7ca4b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1188,25 +1188,18 @@ char * sql_encode_index_opts(struct region *region, const struct index_opts *opts, uint32_t *size) { - 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); - mpstream_encode_map(&stream, 1); - mpstream_encode_str(&stream, "unique"); - mpstream_encode_bool(&stream, opts->is_unique);; - mpstream_flush(&stream); - if (is_error) { - diag_set(OutOfMemory, stream.pos - stream.buf, "mpstream_flush", - "stream"); + int unique_len = strlen("unique"); + *size = mp_sizeof_map(1) + mp_sizeof_str(unique_len) + + mp_sizeof_bool(opts->is_unique); + char *mem = (char *) region_alloc(region, *size); + if (mem == NULL) { + diag_set(OutOfMemory, *size, "region_alloc", "mem"); return NULL; } - *size = region_used(region) - used; - char *raw = region_join(region, *size); - if (raw == NULL) - diag_set(OutOfMemory, *size, "region_join", "raw"); - return raw; + char *pos = mp_encode_map(mem, 1); + pos = mp_encode_str(pos, "unique", unique_len); + pos = mp_encode_bool(pos, opts->is_unique); + return mem; } void diff --git a/src/box/sql/build.c b/src/box/sql/build.c index ab2a56420..50227232c 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2125,7 +2125,6 @@ table_add_index(struct space *space, struct index *index) * @param idx_type Index type: non-unique index, unique index, * index implementing UNIQUE constraint or * index implementing PK constraint. - * @param sql_stmt SQL statement, which creates the index. * @retval 0 on success, -1 on error. */ static int