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 6EE5E26CE0 for ; Fri, 29 Jun 2018 16:46:36 -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 mql4xTT9xgkg for ; Fri, 29 Jun 2018 16:46:36 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 9158026A19 for ; Fri, 29 Jun 2018 16:46:32 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v6] sql: add index_def to struct Index References: <9A0A687D-2E0B-4AB9-B223-1A1287817824@tarantool.org> <8900ae2a-59e8-d69a-7f5d-95436b23214a@tarantool.org> <0e795fb5-1ee8-e7e6-65e4-7347787f3248@tarantool.org> <898ec1ac-51b9-13a6-6298-3a20eeac5b2b@tarantool.org> <76c1ec30-4f33-bc91-8845-72a99fc4ef0f@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6fbc3849-a204-6cd9-82cd-2fb22769ccf0@tarantool.org> Date: Fri, 29 Jun 2018 23:46:28 +0300 MIME-Version: 1.0 In-Reply-To: <76c1ec30-4f33-bc91-8845-72a99fc4ef0f@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Ivan Koptelov , tarantool-patches@freelists.org Hello. Thanks for the fixes! See my 6 comments below. And I have pushed more fixes on the branch. Please, look and squash. > Now every sqlite struct Index is created with tnt struct > index_def inside. This allows us to use tnt index_def > in work with sqlite indexes in the same manner as with > tnt index and is a step to remove sqlite Index with > tnt index. > Fields coll_array, coll_id_array, aiColumn, sort_order > and zName are removed from Index. All usages of this > fields changed to usage of corresponding index_def > fields. > index_is_unique(), sql_index_collation() and > index_column_count() are removed with calls of > index_def corresponding fields. > > Closes: #3369 > > --- > Branch: > https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where > Issue:https://github.com/tarantool/tarantool/issues/3369 > >  src/box/sql.c                        |  54 ++- >  src/box/sql/analyze.c                |  85 ++--- >  src/box/sql/build.c                  | 713 +++++++++++++++++------------------ >  src/box/sql/delete.c                 |  10 +- >  src/box/sql/expr.c                   |  61 +-- >  src/box/sql/fkey.c                   | 132 +++---- >  src/box/sql/insert.c                 | 145 ++++--- >  src/box/sql/pragma.c                 |  30 +- >  src/box/sql/select.c                 |   2 +- >  src/box/sql/sqliteInt.h              | 111 ++---- >  src/box/sql/update.c                 |  39 +- >  src/box/sql/vdbeaux.c                |   2 +- >  src/box/sql/vdbemem.c                |  21 +- >  src/box/sql/where.c                  | 180 ++++----- >  src/box/sql/wherecode.c              | 102 ++--- >  test/sql-tap/colname.test.lua        |   4 +- >  test/sql/message-func-indexes.result |   8 +- >  17 files changed, 821 insertions(+), 878 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index 11353150e..24e37652e 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1452,8 +1452,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf) > >      /* If table's PK is single column which is INTEGER, then >       * treat it as strict type, not affinity.  */ > -    if (pk_idx && pk_idx->nColumn == 1) { > -        int pk = pk_idx->aiColumn[0]; > +    if (pk_idx != NULL && pk_idx->def->key_def->part_count == 1) { > +        int pk = pk_idx->def->key_def->parts[0].fieldno; 1. You again sent the patch with spaces instead of tabs. Please, cope with it. Looks like you copied 'git diff/show' output. Either use format-patch or use 'git --no-pager diff/show'. >          if (def->fields[pk].type == FIELD_TYPE_INTEGER) >              pk_forced_int = pk; >      } > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0da7d805b..662fc698e 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c> @@ -2646,18 +2535,154 @@ addIndexToTable(Index * pIndex, Table * pTab)> +        struct Expr *column_expr = sqlite3ExprSkipCollate(expr); > +        if (column_expr->op != TK_COLUMN) { > +            sqlite3ErrorMsg(parse, tnt_errcode_desc(ER_UNSUPPORTED), > +                        "Tarantool", "functional indexes"); 2. Patch to allow SQL_TARANTOOL_ERROR has been pushed today, so you can use here diag_set again. > +            goto tnt_error; > +        } > + > @@ -2805,108 +2828,92 @@ sql_create_index(struct Parse *parse, struct Token *token, 3. Crash: box.cfg{} box.sql.execute('CREATE TABLE test (a int, b int, c int, PRIMARY KEY (a, a COLLATE kek, b, c))') Process 15886 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x80) frame #0: 0x000000010035f77b tarantool`sql_create_index(parse=0x000000010481f8b0, token=0x0000000000000000, tbl_name=0x0000000000000000, col_list=0x00000001029039d8, on_error=ON_CONFLICT_ACTION_DEFAULT, start=0x0000000000000000, where=0x0000000000000000, sort_order=SORT_ORDER_ASC, if_not_exist=false, idx_type='\x02') at build.c:2895 2892 * PRIMARY KEY contains no repeated columns. 2893 */ 2894 if (IsPrimaryKeyIndex(index)) { -> 2895 struct key_part *parts = index->def->key_def->parts; 2896 uint32_t part_count = index->def->key_def->part_count; 2897 uint32_t new_part_count = 1; 2898 Target 0: (tarantool) stopped. > @@ -3070,54 +3080,17 @@ sql_create_index(struct Parse *parse, struct Token *token, > -/** > - * Return number of columns in given index. > - * If space is ephemeral, use internal > - * SQL structure to fetch the value. > - */ > -uint32_t > -index_column_count(const Index *idx) > -{ > -    assert(idx != NULL); > -    uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->tnum); > -    struct space *space = space_by_id(space_id); > -    /* It is impossible to find an ephemeral space by id. */ > -    if (space == NULL) > -        return idx->nColumn; > - > -    uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum); > -    struct index *index = space_index(space, index_id); > -    assert(index != NULL); > -    return index->def->key_def->part_count; > -} > - > -/** Return true if given index is unique and not nullable. */ > -bool > -index_is_unique_not_null(const Index *idx) 4. Same as on the previous review. Still is used in a pair of places. > -{ > -    assert(idx != NULL); > -    uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->tnum); > -    struct space *space = space_by_id(space_id); > -    assert(space != NULL); > - > -    uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum); > -    struct index *index = space_index(space, index_id); > -    assert(index != NULL); > -    return (index->def->opts.is_unique && > -        !index->def->key_def->is_nullable); > +    sqlite3DbFree(db, name); >  } > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index c0c26ce29..225fddc23 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -2913,11 +2898,32 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder,    /* WHERE clause information */ >           */ >          Index *pFirst;    /* First of real indices on the table */ >          memset(&sPk, 0, sizeof(Index)); > -        sPk.nColumn = 1; > -        sPk.aiColumn = &aiColumnPk; >          sPk.aiRowLogEst = aiRowEstPk; >          sPk.onError = ON_CONFLICT_ACTION_REPLACE; >          sPk.pTable = pTab; > + > +        struct key_def *key_def = key_def_new(1); > +        if (key_def == NULL) { > +            pWInfo->pParse->nErr++; > +            pWInfo->pParse->rc = SQL_TARANTOOL_ERROR; > +            return SQL_TARANTOOL_ERROR; > +        } > + > +        key_def_set_part(key_def, 0, 0, pTab->def->fields[0].type, > +                 ON_CONFLICT_ACTION_ABORT, > +                 NULL, COLL_NONE, SORT_ORDER_ASC); > + > +        sPk.def = index_def_new(pTab->def->id, 0, "primary", > +                    sizeof("primary") - 1, TREE, > +                    &index_opts_default, key_def, NULL); >> 11. Where is sPk.def is deleted? > It is deleted in freeIndex() with sPk 5. I do not see any mention of freeIndex() in where.c. Where is it deleted? sPk is declared on stack. If it was deleted with freeIndex, Tarantool would crash. > +        key_def_delete(key_def); > + > +        if (sPk.def == NULL) { > +            pWInfo->pParse->nErr++; > +            pWInfo->pParse->rc = SQL_TARANTOOL_ERROR; > +            return SQL_TARANTOOL_ERROR; > +        } > + >          aiRowEstPk[0] = sql_space_tuple_log_count(pTab); >          aiRowEstPk[1] = 0; >          pFirst = pSrc->pTab->pIndex; 6. Where is the test, that I gave you on the previous review and that lead to crash? Please, add it to the test suite. And the new test in this review too.