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 2569928A99 for ; Mon, 13 Aug 2018 16:24:33 -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 kgU8dq0HSibp for ; Mon, 13 Aug 2018 16:24:33 -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 C619C28809 for ; Mon, 13 Aug 2018 16:24:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 05/10] sql: remove affinity string of columns from Index References: <026cd8e04034ad26529a2b8e76e1ae9aabf9076f.1534001739.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9ad38333-305b-7c55-a682-6b473d76e4f4@tarantool.org> Date: Mon, 13 Aug 2018 23:24:30 +0300 MIME-Version: 1.0 In-Reply-To: <026cd8e04034ad26529a2b8e76e1ae9aabf9076f.1534001739.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! I have pushed my review fixes in a separate commit. Also see 3 comments below. On 12/08/2018 17:13, Nikita Pettik wrote: > Part of #3561 > --- > src/box/sql/build.c | 1 - > src/box/sql/delete.c | 3 ++- > src/box/sql/insert.c | 42 +++++++++++------------------------------- > src/box/sql/sqliteInt.h | 16 +++++++++++----- > src/box/sql/update.c | 2 +- > src/box/sql/vdbemem.c | 3 +-- > src/box/sql/where.c | 21 ++++++++------------- > src/box/sql/wherecode.c | 3 ++- > 8 files changed, 36 insertions(+), 55 deletions(-) > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 0be68830a..cb16a7c0d 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -352,7 +352,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > key_len = 0; > struct Index *pk = sqlite3PrimaryKeyIndex(table); > const char *zAff = is_view ? NULL : > - sqlite3IndexAffinityStr(parse->db, pk); > + sql_index_affinity_str(parse->db, > + pk->def); 1. sql_index_affinity_str makes extra lookup in space cache. Here (in sql_table_delete_from) space ptr is known already. Same in fkey_lookup_parent. Maybe it would be better to pass space ptr alongside with index_def? And name it sql_space_index_affinity_str. > sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len, > reg_key, zAff, pk_len); > /* Set flag to save memory allocating one > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index ddd7f932b..2b6100ad5 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -112,15 +84,23 @@ sql_index_affinity_str(struct sqlite3 *db, struct index_def *def) > return NULL; > struct space *space = space_by_id(def->space_id); > assert(space != NULL); > - > + /* > + * Table may occasionally come from Lua, so lets > + * gentle process this case by setting default > + * affinity for it. > + */ > + if (space->def->fields == NULL) { > + memset(aff, AFFINITY_BLOB, column_count); > + goto exit; > + } > for (uint32_t i = 0; i < column_count; i++) { > uint32_t x = def->key_def->parts[i].fieldno; > aff[i] = space->def->fields[x].affinity; > if (aff[i] == AFFINITY_UNDEFINED) > - aff[i] = 'A'; > + aff[i] = AFFINITY_BLOB; > } > +exit: 2. Please, try to avoid extra labels when possible. They might help only when there are many error processing code snippets or when it allows to strongly reduce indentation. Otherwise it slightly confuses. I have fixed it on the branch. > aff[column_count] = '\0'; > - > return aff; > } > > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index db8d0bf80..794db0302 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -1224,7 +1220,7 @@ whereRangeSkipScanEst(Parse * pParse, /* Parsing & code generating context */ > int nLower = -1; > int nUpper = index->def->opts.stat->sample_count + 1; > int rc = SQLITE_OK; > - u8 aff = sqlite3IndexColumnAffinity(db, p, nEq); > + u8 aff = sql_index_part_affinity(p->def, nEq); 3. Same as 1 - extra lookup in the space cache. You have the space few lines above. > > sqlite3_value *p1 = 0; /* Value extracted from pLower */ > sqlite3_value *p2 = 0; /* Value extracted from pUpper */