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 12D002A60E for ; Mon, 27 Aug 2018 21:43:07 -0400 (EDT) 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 YsKUsjCNRHwz for ; Mon, 27 Aug 2018 21:43:06 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 651B429D14 for ; Mon, 27 Aug 2018 21:43:06 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: remove struct Enc References: <388a4c514161fb19233e3185e6f211d33fd7f7f3.1535367103.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 27 Aug 2018 22:43:00 -0300 MIME-Version: 1.0 In-Reply-To: <388a4c514161fb19233e3185e6f211d33fd7f7f3.1535367103.git.kshcherbatov@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: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! See 7 comments below and a commit on the branch. On 27/08/2018 08:11, Kirill Shcherbatov wrote: > We shouldn't use legacy Enc structure in SQL as we have > featured class mpstream in tarantool core. With this path > all allocations temporally commited with mpstream initialized > on region and then duplicated in dynamic persistent memory via > sqlite3DbMallocRaw. > > Closes #3545. > --- > src/box/sql.c | 473 +++++++++++++++++++++++---------------------- > src/box/sql/build.c | 156 ++++++++------- > src/box/sql/tarantoolInt.h | 80 +++++--- > src/box/sql/trigger.c | 62 +++--- > 4 files changed, 416 insertions(+), 355 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index b98593b..6bf832b 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -56,6 +56,8 @@ > #include "xrow.h" > #include "iproto_constants.h" > #include "fkey.h" > +#include "mpstream.h" > +#include "small/region.h" 1. Double include of region.h. > > static sqlite3 *db = NULL; > > @@ -840,6 +842,32 @@ rename_fail: > return SQL_TARANTOOL_ERROR; > } > > +/** Callback to forward and error from mpstream methods. */ > +static void > +set_encode_error(void *error_ctx) > +{ > + *(bool *)error_ctx = true; > +} > + > +void > +mpstream_encode_index_opts(struct mpstream *stream, struct index_opts *opts) 2. Static. 3. No comment. > +{ > + mpstream_encode_map(stream, 2); > + /* Mark as unique pk and unique indexes */ 4. Finish a sentence with the dot. Btw the comment is not related to the code. Below you do not whether the index is unique or not. > + mpstream_encode_str(stream, "unique"); > + /* If user didn't defined ON CONFLICT OPTIONS, all uniqueness checks > + * will be made by Tarantool. However, Tarantool doesn't have ON > + * CONFLIT option, so in that case (except ON CONFLICT ABORT, which is > + * default behavior) uniqueness will be checked by SQL. > + * INSERT OR REPLACE/IGNORE uniqueness checks will be also done by > + * Tarantool. > + */ 5. "Did't defined" - corrupted phrase. Rather "did't define". But the whole comment looks strange as well as the previous, and out of its place. Just remove it. > + mpstream_encode_bool(stream, opts->is_unique); > + mpstream_encode_str(stream, "sql"); > + mpstream_encode_strn(stream, opts->sql, > + opts->sql != NULL ? strlen(opts->sql) : 0);> +} > + > int > sql_index_update_table_name(struct index_def *def, const char *new_tbl_name, > char **sql_stmt) > @@ -851,33 +879,43 @@ sql_index_update_table_name(struct index_def *def, const char *new_tbl_name, > if (*sql_stmt == NULL) > return -1; > > - uint32_t key_len = mp_sizeof_uint(def->space_id) + > - mp_sizeof_uint(def->iid) + mp_sizeof_array(2); > + 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, 2); > + mpstream_encode_uint(&stream, def->space_id); > + mpstream_encode_uint(&stream, def->iid); > + > + /* Encode op. */ > + 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_INDEX_FIELD_OPTS); > + > + /* Encode index opts. */ > struct index_opts opts = def->opts; > opts.sql = *sql_stmt; > - uint32_t new_opts_sz = sql_encode_index_opts(&opts, NULL); > - uint32_t op_sz = mp_sizeof_array(1) + mp_sizeof_array(3) + > - mp_sizeof_str(1) + mp_sizeof_uint(4) + new_opts_sz; > + mpstream_encode_index_opts(&stream, &opts); > > - char *key_begin = (char*) region_alloc(&fiber()->gc, key_len + op_sz); > - if (key_begin == NULL) { > - diag_set(OutOfMemory, key_len + op_sz, "region_alloc", > - "key_begin"); > + mpstream_flush(&stream); > + if (is_error) { > + diag_set(OutOfMemory, stream.pos - stream.buf, > + "mpstream_flush", "stream"); > return -1; > } > - char *key = mp_encode_array(key_begin, 2); > - key = mp_encode_uint(key, def->space_id); > - key = mp_encode_uint(key, def->iid); > - > - char *op_begin = key; > - char *op = mp_encode_array(op_begin, 1); > - op = mp_encode_array(op, 3); > - op = mp_encode_str(op, "=", 1); > - op = mp_encode_uint(op, BOX_INDEX_FIELD_OPTS); > - op += sql_encode_index_opts(&opts, op); > - > - if (box_update(BOX_INDEX_ID, 0, key_begin, key, op_begin, op, > - 0, NULL) != 0) > + size_t sz = region_used(region) - used; > + char *raw = region_join(region, sz); > + if (raw == NULL) > + diag_set(OutOfMemory, sz, "region_join", "raw"); > + > + if (box_update(BOX_INDEX_ID, 0, raw, raw + op_offset, raw + op_offset, 6. If 'raw == NULL' you still try to use it in box_update. > + raw + sz, 0, NULL) != 0) > return -1; > > return 0; 7. Here https://github.com/tarantool/tarantool/commit/0d8b0c5641ec4c27547b1d0121bb1e0a641b149b I see changes not relevant to this patch and not presented in the email. I paste them bellow: > diff --git a/src/mpstream.c b/src/mpstream.c > index 23d27f9e5..44c2d9213 100644 > --- a/src/mpstream.c > +++ b/src/mpstream.c > @@ -176,12 +176,6 @@ mpstream_encode_strn(struct mpstream *stream, const char *str, uint32_t len) > mpstream_advance(stream, pos - data); > } > > -void > -mpstream_encode_str(struct mpstream *stream, const char *str) > -{ > - mpstream_encode_strn(stream, str, strlen(str)); > -} > - > void > mpstream_encode_nil(struct mpstream *stream) > { > diff --git a/src/mpstream.h b/src/mpstream.h > index 789bd741d..6f9462d4c 100644 > --- a/src/mpstream.h > +++ b/src/mpstream.h > @@ -108,8 +108,11 @@ mpstream_encode_double(struct mpstream *stream, double num); > void > mpstream_encode_strn(struct mpstream *stream, const char *str, uint32_t len); > > -void > -mpstream_encode_str(struct mpstream *stream, const char *str); > +static inline void > +mpstream_encode_str(struct mpstream *stream, const char *str) > +{ > + mpstream_encode_strn(stream, str, strlen(str)); > +} > > void > mpstream_encode_nil(struct mpstream *stream); Obviously, it should not be part of this comment, but of the previous one. Below and on the branch you can find my fixes: diff --git a/src/box/sql.c b/src/box/sql.c index 6bf832bd7..d1b935c14 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -57,7 +57,6 @@ #include "iproto_constants.h" #include "fkey.h" #include "mpstream.h" -#include "small/region.h" static sqlite3 *db = NULL; @@ -911,14 +910,12 @@ sql_index_update_table_name(struct index_def *def, const char *new_tbl_name, } size_t sz = region_used(region) - used; char *raw = region_join(region, sz); - if (raw == NULL) + if (raw == NULL) { diag_set(OutOfMemory, sz, "region_join", "raw"); - - if (box_update(BOX_INDEX_ID, 0, raw, raw + op_offset, raw + op_offset, - raw + sz, 0, NULL) != 0) return -1; - - return 0; + } + return box_update(BOX_INDEX_ID, 0, raw, raw + op_offset, + raw + op_offset, raw + sz, 0, NULL); } int