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 320B8264A7 for ; Thu, 28 Jun 2018 14:49:47 -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 OClc8RLZ3Jdq for ; Thu, 28 Jun 2018 14:49:47 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 6630D264A6 for ; Thu, 28 Jun 2018 14:49:46 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5] 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> From: Vladislav Shpilevoy Message-ID: Date: Thu, 28 Jun 2018 21:49:38 +0300 MIME-Version: 1.0 In-Reply-To: <898ec1ac-51b9-13a6-6298-3a20eeac5b2b@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 Cc: Ivan Koptelov Hello. Thanks for the patch! See 11 comments below. Besides, see some of them and others in the separate commit on the branch. Please, look at it and squash. Note: I did not run the tests, so please, repair them if they fail. > commit cba31099432e47a4f65d5d48280da2608d6e615d > Author: Ivan Koptelov > Date: Fri Jun 8 10:32:01 2018 +0300 > > sql: add index_def to Index > > 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 > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0da7d805b..2c82644e6 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1049,22 +1052,20 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken) > uint32_t *id = &p->def->fields[i].coll_id; > p->aCol[i].coll = sql_get_coll_seq(pParse, zColl, id); > if (p->aCol[i].coll != NULL) { > - Index *pIdx; > /* If the column is declared as " PRIMARY KEY COLLATE ", > * then an index may have been created on this column before the > * collation type was added. Correct this if it is the case. > */ > - for (pIdx = p->pIndex; pIdx; pIdx = pIdx->pNext) { > - assert(pIdx->nColumn == 1); > - if (pIdx->aiColumn[0] == i) { > - id = &pIdx->coll_id_array[0]; > - pIdx->coll_array[0] = > + for (struct Index *pIdx = p->pIndex; pIdx; pIdx = pIdx->pNext) { > + assert(pIdx->def->key_def->part_count == 1); > + if (pIdx->def->key_def->parts[0].fieldno == i) { > + pIdx->def->key_def->parts[0].coll_id = *id; 1. Here you should set id to address of parts[0].coll_id so that it will be initialized in sql_column_collation(). Now you set coll_id to id before initialization of id (fixed by me on the branch). > + pIdx->def->key_def->parts[0].coll = > sql_column_collation(p->def, i, id); > } > } > - } else { > - sqlite3DbFree(db, zColl); > } > + sqlite3DbFree(db, zColl); > } 2. I caught a crash: box.cfg{} box.sql.execute('CREATE TABLE test (a int, b int, c int, PRIMARY KEY (a, a, b, c))') Process 85357 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38) frame #0: 0x000000010036122f tarantool`convertToWithoutRowidTable(pParse=0x000000010401f8b0, pTab=0x0000000104600218) at build.c:1365 1362 * PRIMARY KEY contains no repeated columns. 1363 */ 1364 -> 1365 struct key_part *parts = pPk->def->key_def->parts; 1366 uint32_t part_count = pPk->def->key_def->part_count; 1367 uint32_t new_part_count = part_count; 1368 Target 0: (tarantool) stopped > @@ -1404,18 +1355,33 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) > pPk = sqlite3PrimaryKeyIndex(pTab); > > /* > - * Remove all redundant columns from the PRIMARY KEY. For example, change > - * "PRIMARY KEY(a,b,a,b,c,b,c,d)" into just "PRIMARY KEY(a,b,c,d)". Later > - * code assumes the PRIMARY KEY contains no repeated columns. > + * Remove all redundant columns from the PRIMARY > + * KEY. For example, change > + * "PRIMARY KEY(a,b,a,b,c,b,c,d)" into just > + * "PRIMARY KEY(a,b,c,d)". Later code assumes the > + * PRIMARY KEY contains no repeated columns. > */ > - for (i = j = 1; i < pPk->nColumn; i++) { > - if (hasColumn(pPk->aiColumn, j, pPk->aiColumn[i])) { > - pPk->nColumn--; > - } else { > - pPk->aiColumn[j++] = pPk->aiColumn[i]; > + > + struct key_part *parts = pPk->def->key_def->parts; > + uint32_t part_count = pPk->def->key_def->part_count; > + uint32_t new_part_count = part_count; > + > + for (uint32_t i = 1; i < part_count; i++) { > + if (is_part_duplicated(parts, i)) { > + new_part_count--; > + bool is_found = false; > + for (uint32_t j = i + 1; j < part_count; j++) { > + if (!is_part_duplicated(parts, j)) { > + parts[i] = parts[j]; > + is_found = true; > + break; > + } > + } > + if (!is_found) > + break; > } > } 3. This cycle and is_part_duplicated still are useless non working things. Example: CREATE TABLE test (a int, b int, c int, PRIMARY KEY (a, a, a, a)) Here the primary index has duplicate 'a'. Its key_def has these fieldnos: [0, 0, 0, 0]. In your cycle you will update this def to [0, 0, 0], it is not? So there is still 3 duplicates. And please, remove is_part_duplicated and use key_def_find. > - pPk->nColumn = j; > + pPk->def->key_def->part_count = new_part_count; > } > assert(pPk != 0); > } > @@ -2531,34 +2499,20 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage) > */ > Index * > sqlite3AllocateIndexObject(sqlite3 * db, /* Database connection */ > - i16 nCol, /* Total number of columns in the index */ > - int nExtra, /* Number of bytes of extra space to alloc */ > - char **ppExtra /* Pointer to the "extra" space */ > + i16 nCol /* Total number of columns in the index */ 4. You have removed more than half of the function, so it is time to convert it to Tarantool code style. (fixed by me on the branch). > ) > { > Index *p; /* Allocated index object */ > int nByte; /* Bytes of space for Index object + arrays */ > > nByte = ROUND8(sizeof(Index)) + /* Index structure */ > - ROUND8(sizeof(struct coll *) * nCol) + /* Index.coll_array */ > - ROUND8(sizeof(uint32_t) * nCol) + /* Index.coll_id_array*/ > - ROUND8(sizeof(LogEst) * (nCol + 1) + /* Index.aiRowLogEst */ > - sizeof(i16) * nCol + /* Index.aiColumn */ > - sizeof(enum sort_order) * nCol); /* Index.sort_order */ > - p = sqlite3DbMallocZero(db, nByte + nExtra); > + ROUND8(sizeof(LogEst) * (nCol + 1)); /* Index.aiRowLogEst */ > + p = sqlite3DbMallocZero(db, nByte); > if (p) { > char *pExtra = ((char *)p) + ROUND8(sizeof(Index)); > - p->coll_array = (struct coll **)pExtra; > - pExtra += ROUND8(sizeof(struct coll **) * nCol); > - p->coll_id_array = (uint32_t *) pExtra; > - pExtra += ROUND8(sizeof(uint32_t) * nCol); > p->aiRowLogEst = (LogEst *) pExtra; > pExtra += sizeof(LogEst) * (nCol + 1); > - p->aiColumn = (i16 *) pExtra; > pExtra += sizeof(i16) * nCol; 5. You do not need pExtra propagation. It is not used below. (fixed by me on the branch). > - p->sort_order = (enum sort_order *) pExtra; > - p->nColumn = nCol; > - *ppExtra = ((char *)p) + nByte; > } > return p; > } > @@ -2646,18 +2600,142 @@ addIndexToTable(Index * pIndex, Table * pTab) 6. Please, add a comment here. > +static void > +set_index_def(Parse *parse, Index *index, Table *table, uint32_t iid, > + const char *name, uint32_t name_len, int on_error, > + struct ExprList *expr_list, u8 idx_type) > diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c > index e3fff37fe..c14a70836 100644 > --- a/src/box/sql/fkey.c > +++ b/src/box/sql/fkey.c > @@ -287,9 +294,16 @@ sqlite3FkLocateIndex(Parse * pParse, /* Parse context to store any error in */ > * the default collation sequences for each column. > */ > int i, j; > - for (i = 0; i < nCol; i++) { > - i16 iCol = pIdx->aiColumn[i]; /* Index of column in parent tbl */ > - char *zIdxCol; /* Name of indexed column */ > + struct key_part *part = > + index->def->key_def->parts; > + for (i = 0; i < nCol; i++, part++) { > + /* > + * Index of column in > + * parent table. > + * */ > + i16 iCol = (int) part->fieldno; > + /* Name of indexed column. */ > + char *zIdxCol; > > if (iCol < 0) 7. How can iCol be < 0, if it was get from uint fieldno? > diff --git a/src/box/sql/where.c b/src/box/sql/where.c > index c0c26ce29..599863041 100644 > --- a/src/box/sql/where.c > +++ b/src/box/sql/where.c > @@ -2523,14 +2505,16 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder, /* The WhereLoop factory */ > */ > } > } else if (eOp & WO_EQ) { > - int iCol = pProbe->aiColumn[saved_nEq]; > + int iCol = pProbe->def->key_def->parts[saved_nEq].fieldno; > pNew->wsFlags |= WHERE_COLUMN_EQ; > assert(saved_nEq == pNew->nEq); > - if ((iCol > 0 && nInMul == 0 > - && saved_nEq == nProbeCol - 1) > - ) { > - if (iCol >= 0 && > - !index_is_unique_not_null(pProbe)) { 8. This function is still declared (with not implementation) and is even used in a couple of places. > @@ -2913,11 +2896,28 @@ 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) > + return SQLITE_ERROR; 9. Why SQLITE_ERROR. AFAIR we have decided to use SQL_TARANTOOL_ERROR + nErr + rc setting. > + > + key_def_set_part(key_def, 0, 0, pTab->def->fields[0].type, > + ON_CONFLICT_ACTION_ABORT, > + NULL, COLL_NONE, SORT_ORDER_ASC); > + > + struct index_opts index_opts = index_opts_default; > + > + sPk.def = index_def_new(pTab->def->id, 0, "primary", > + sizeof("primary") - 1, TREE, &index_opts, > + key_def, NULL); > + key_def_delete(key_def); > + > + if (sPk.def == NULL) > + return SQLITE_ERROR; 10. Same. 11. Where is sPk.def is deleted? > + > aiRowEstPk[0] = sql_space_tuple_log_count(pTab); > aiRowEstPk[1] = 0; > pFirst = pSrc->pTab->pIndex;