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 861BE2A99B for ; Thu, 6 Sep 2018 15:54:32 -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 llyhVVlM0b7T for ; Thu, 6 Sep 2018 15:54:32 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 E86952A98A for ; Thu, 6 Sep 2018 15:54:31 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/7] sql: remove SQLite original struct Index References: <282cdd4662d2195250c1e9a828fde201d4822974.1535064700.git.korablev@tarantool.org> <0bd856e4-2178-cfa9-b680-fe1d47ce16c7@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 6 Sep 2018 22:54:27 +0300 MIME-Version: 1.0 In-Reply-To: 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: tarantool-patches@freelists.org, "n.pettik" Hi! Thanks for the fixes! See my own ones on the branch and comments below. >>> + assert(space != NULL); >>> + for (uint32_t i = 0; i < space->index_count; ++i) { >>> + struct index *idx = space->index[i]; >> >> 2. You have mentioned that space indexes are stored as an >> array, but it is not the only true. The indexes are stored in >> two ways: as an array and a map. A map allows to find an index >> by id in O(1) time. Look at space_index function. > > It is cool and I know about it, but the most common use case is simple > iteration over all indexes like: > for (i = 0; i < space->index_count; ++i) { > struct index *idx = space->index[I]; > ... > }> >> Here you do not clear space->index_map and do not >> reallocate it. Looks like you do not touch index_map >> at all in this patch. But maybe you should. See my >> other comments here and in next patches. > > I deliberately didn’t fill in index map. I think maintaining > map and array at the same time seems to be over-engineering. > These indexes are really used only during table creation > (after last patch in series). Moreover, during building routine > we may need only PK index, which anyway can be fetched as index[0] It is not over-engineering. This is how real spaces work and I want these surrogate spaces be as much similar as possible to them. So I implemented index_map support in my review fixes. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index d1d952330..617d0ea47 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -260,38 +252,39 @@ table_column_is_in_pk(Table *table, uint32_t column) > return false; > } > > -/* > +/** > * Remove the memory data structures associated with the given > - * Table. No changes are made to disk by this routine. > - * > - * This routine just deletes the data structure. It does not unlink > - * the table data structure from the hash table. But it does destroy > - * memory structures of the indices and foreign keys associated with > - * the table. > + * Table. > * > - * The db parameter is optional. It is needed if the Table object > - * contains lookaside memory. (Table objects in the schema do not use > - * lookaside memory, but some ephemeral Table objects do.) Or the > - * db parameter can be used with db->pnBytesFreed to measure the memory > - * used by the Table object. > + * @param db Database handler. > + * @param tab Table to be deleted. > */ > -static void SQLITE_NOINLINE > -deleteTable(sqlite3 * db, Table * pTable) > +static void > +table_delete(struct sqlite3 *db, struct Table *tab) > { > - Index *pIndex, *pNext; > - > + if (tab->space->def != NULL) > + goto skip_index_delete; > /* Delete all indices associated with this table. */ > - for (pIndex = pTable->pIndex; pIndex; pIndex = pNext) { > - pNext = pIndex->pNext; > - freeIndex(db, pIndex); > - } > - assert(pTable->def != NULL); > - /* Do not delete pTable->def allocated on region. */ > - if (!pTable->def->opts.is_temporary) > - space_def_delete(pTable->def); > + for (uint32_t i = 0; i < tab->space->index_count; ++i) { > + /* > + * These indexes are just wrapper for > + * index_def's, so it makes no sense to call > + * index_delete(). > + */ > + struct index *idx = tab->space->index[i]; > + free(idx->def); Leak of idx->def->key_def/cmp_def. > + free(idx); > + } > + free(tab->space->index); > + free(tab->space); > +skip_index_delete: > + assert(tab->def != NULL); > + /* Do not delete table->def allocated on region. */ > + if (!tab->def->opts.is_temporary) > + space_def_delete(tab->def); > else > - sql_expr_list_delete(db, pTable->def->opts.checks); > - sqlite3DbFree(db, pTable); > + sql_expr_list_delete(db, tab->def->opts.checks); > + sqlite3DbFree(db, tab); > } > > void > @@ -1156,137 +1137,132 @@ getNewSpaceId(Parse * pParse) > return iRes; > } > > -/* > - * Generate VDBE code to create an Index. This is acomplished by adding > - * an entry to the _index table. ISpaceId either contains the literal > - * space id or designates a register storing the id. > +/** > + * Generate VDBE code to create an Index. This is accomplished by > + * adding an entry to the _index table. > + * > + * @param parse Current parsing context. > + * @param def Definition of space which index belongs to. > + * @param idx_def Definition of index under construction. > + * @param pk_def Definition of primary key index. > + * @param space_id_reg Register containing generated space id. > */ > static void > -createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId, > - const char *zSql) > +vdbe_emit_create_index(struct Parse *parse, struct space_def *def, > + const struct index_def *idx_def, > + const struct index_def *pk_def, int space_id_reg) > { > - Vdbe *v = sqlite3GetVdbe(pParse); > - int iFirstCol = ++pParse->nMem; > - int iRecord = (pParse->nMem += 6); /* 6 total columns */ > - struct index_opts opts = pIndex->def->opts; > - opts.sql = (char *)zSql; > - struct region *region = &pParse->region; > + struct Vdbe *v = sqlite3GetVdbe(parse); > + int entry_reg = ++parse->nMem; > + /* > + * Entry in _index space contains 6 fields. > + * The last one contains encoded tuple. > + */ > + int tuple_reg = (parse->nMem += 6); > + /* Format "opts" and "parts" for _index entry. */ > + struct region *region = &parse->region; > uint32_t index_opts_sz = 0; > - char *index_opts = > - sql_encode_index_opts(region, &opts, &index_opts_sz); > + char *index_opts = sql_encode_index_opts(region, &idx_def->opts, > + &index_opts_sz); > if (index_opts == NULL) > goto error; > uint32_t index_parts_sz = 0; > - char *index_parts = > - sql_encode_index_parts(region, pIndex, &index_parts_sz); > + char *index_parts = sql_encode_index_parts(region, def->fields, idx_def, > + pk_def, &index_parts_sz); > if (index_parts == NULL) > goto error; > - char *raw = sqlite3DbMallocRaw(pParse->db, > - index_opts_sz + index_parts_sz); > + char *raw = sqlite3DbMallocRaw(parse->db, > + index_opts_sz +index_parts_sz); > if (raw == NULL) > return; > - > memcpy(raw, index_opts, index_opts_sz); > index_opts = raw; > raw += index_opts_sz; > memcpy(raw, index_parts, index_parts_sz); > index_parts = raw; > > - if (pParse->pNewTable) { > - int reg; > - /* > - * A new table is being created, hence iSpaceId is a register, but > - * iIndexId is literal. > - */ > - sqlite3VdbeAddOp2(v, OP_SCopy, iSpaceId, iFirstCol); > - sqlite3VdbeAddOp2(v, OP_Integer, iIndexId, iFirstCol + 1); > - > - /* Generate code to save new pageno into a register. > - * This is runtime implementation of SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID: > - * pageno = (spaceid << 10) | indexid > - */ > - pParse->regRoot = ++pParse->nMem; > - reg = ++pParse->nMem; > - sqlite3VdbeAddOp2(v, OP_Integer, 1 << 10, reg); > - sqlite3VdbeAddOp3(v, OP_Multiply, reg, iSpaceId, > - pParse->regRoot); > - sqlite3VdbeAddOp3(v, OP_AddImm, pParse->regRoot, iIndexId, > - pParse->regRoot); > + if (parse->pNewTable != NULL) { > + sqlite3VdbeAddOp2(v, OP_SCopy, space_id_reg, entry_reg); > + sqlite3VdbeAddOp2(v, OP_Integer, idx_def->iid, entry_reg + 1); > } else { > /* > - * An existing table is being modified; iSpaceId is literal, but > - * iIndexId is a register. > + * An existing table is being modified; > + * space_id_reg is register, but iid is literal. The comment is wrong. Here space_id_reg is literal and idx_def->iid is a register. And here a problem is hidden. idx_def->iid is initialized above as either 0 for primary indexes or 1 for other. But by a coincidence it matched result of getNewIid() so it worked ok. Once I added index_map and more strict index identifiers calculation, this place fired. It is fixed in my review fixes. > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index efc531bbd..f3e65303b 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -207,6 +207,7 @@ fkey_encode_links(struct region *region, const struct fkey_def *def, int type, > uint32_t *size); > > /** > +<<<<<<< HEAD Ooops. > * Encode index parts of given foreign key constraint into > * MsgPack on @region. > * @param region Region to use. Fixes: ================================================================ commit dbeb4281577df192f1cbbc670070de74c93dea39 Author: Vladislav Shpilevoy Date: Thu Sep 6 20:18:17 2018 +0300 Review fixes diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index e856d64c7..2c54c1835 100644 --- a/extra/mkkeywordhash.c +++ b/extra/mkkeywordhash.c @@ -82,11 +82,6 @@ struct Keyword { #else # define PRAGMA 0x00000400 #endif -#ifdef SQLITE_OMIT_REINDEX -# define REINDEX 0 -#else -# define REINDEX 0x00000800 -#endif #define SUBQUERY 0x00001000 # define TRIGGER 0x00002000 # define VIEW 0x00008000 diff --git a/src/box/sql.c b/src/box/sql.c index 0c1df9b75..97948ab3b 100644 --- a/src/box/sql.c +++ b/src/box/sql.c @@ -1297,7 +1297,7 @@ sql_encode_table(struct region *region, struct Table *table, uint32_t *size) * If table's PK is single column which is INTEGER, then * treat it as strict type, not affinity. */ - struct index *pk_idx = sql_table_primary_key(table); + struct index *pk_idx = space_index(table->space, 0); uint32_t pk_forced_int = UINT32_MAX; if (pk_idx != NULL && pk_idx->def->key_def->part_count == 1) { int pk = pk_idx->def->key_def->parts[0].fieldno; @@ -1659,21 +1659,30 @@ sql_ephemeral_table_new(Parse *parser, const char *name) Table *table = sqlite3DbMallocZero(db, sizeof(Table)); if (table != NULL) def = sql_ephemeral_space_def_new(parser, name); - if (def == NULL) { - sqlite3DbFree(db, table); - return NULL; - } + if (def == NULL) + goto err_def; table->space = (struct space *) calloc(1, sizeof(struct space)); if (table->space == NULL) { diag_set(OutOfMemory, sizeof(struct space), "calloc", "space"); - parser->rc = SQL_TARANTOOL_ERROR; - parser->nErr++; - sqlite3DbFree(db, table); - return NULL; + goto err_space; + } + table->space->index_map = + (struct index **) calloc(1, sizeof(struct index *)); + if (table->space->index_map == NULL) { + diag_set(OutOfMemory, sizeof(struct index *), "calloc", + "table->space->index_map"); + goto err_map; } - table->def = def; return table; +err_map: + free(table->space); +err_space: + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; +err_def: + sqlite3DbFree(db, table); + return NULL; } int diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c index 80293c6ed..1c9a672d4 100644 --- a/src/box/sql/analyze.c +++ b/src/box/sql/analyze.c @@ -802,7 +802,14 @@ analyzeOneTable(Parse * pParse, /* Parser context */ return; } assert(pTab->def->id != 0); - if (sqlite3_strlike("\\_%", pTab->def->name, '\\') == 0) { + /* + * Here we need real space from Tarantool DD since + * further it is passed to cursor opening routine. + */ + struct space *space = space_by_id(pTab->def->id); + assert(space != NULL); + const char *tab_name = space_name(space); + if (sqlite3_strlike("\\_%", tab_name, '\\') == 0) { /* Do not gather statistics on system tables */ return; } @@ -816,15 +823,9 @@ analyzeOneTable(Parse * pParse, /* Parser context */ iIdxCur = iTab++; pParse->nTab = MAX(pParse->nTab, iTab); sqlite3OpenTable(pParse, iTabCur, pTab, OP_OpenRead); - sqlite3VdbeLoadString(v, regTabname, pTab->def->name); - /* - * Here we need real space from Tarantool DD since - * further it is passed to cursor opening routine. - */ - struct space *space = space_by_id(pTab->def->id); - assert(space != NULL); + sqlite3VdbeLoadString(v, regTabname, tab_name); for (uint32_t j = 0; j < space->index_count; ++j) { - struct index *idx = pTab->space->index[j]; + struct index *idx = space->index[j]; int addrRewind; /* Address of "OP_Rewind iIdxCur" */ int addrNextRow; /* Address of "next_row:" */ const char *idx_name; /* Name of the index */ @@ -834,15 +835,14 @@ analyzeOneTable(Parse * pParse, /* Parser context */ * instead more familiar table name. */ if (idx->def->iid == 0) - idx_name = pTab->def->name; + idx_name = tab_name; else idx_name = idx->def->name; int part_count = idx->def->key_def->part_count; /* Populate the register containing the index name. */ sqlite3VdbeLoadString(v, regIdxname, idx_name); - VdbeComment((v, "Analysis for %s.%s", pTab->def->name, - idx_name)); + VdbeComment((v, "Analysis for %s.%s", tab_name, idx_name)); /* * Pseudo-code for loop that calls stat_push(): @@ -986,7 +986,7 @@ analyzeOneTable(Parse * pParse, /* Parser context */ * if !eof(csr) goto next_row; */ assert(regKey == (regStat4 + 2)); - struct index *pk = sql_table_primary_key(pTab); + struct index *pk = space_index(space, 0); int pk_part_count = pk->def->key_def->part_count; /* Allocate memory for array. */ pParse->nMem = MAX(pParse->nMem, @@ -994,10 +994,10 @@ analyzeOneTable(Parse * pParse, /* Parser context */ int regKeyStat = regPrev + part_count; for (i = 0; i < pk_part_count; i++) { uint32_t k = pk->def->key_def->parts[i].fieldno; - assert(k < pTab->def->field_count); + assert(k < space->def->field_count); sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, k, regKeyStat + i); - VdbeComment((v, "%s", pTab->def->fields[k].name)); + VdbeComment((v, "%s", space->def->fields[k].name)); } sqlite3VdbeAddOp3(v, OP_MakeRecord, regKeyStat, pk_part_count, regKey); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 617d0ea47..d708dbeda 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -159,40 +159,52 @@ sqlite3LocateIndex(sqlite3 * db, const char *zName, const char *zTable) return NULL; } -void +int sql_space_index_delete(struct space *space, uint32_t iid) { assert(space != NULL); + struct index *to_delete = NULL; + uint32_t new_index_id_max = 0; for (uint32_t i = 0; i < space->index_count; ++i) { - struct index *idx = space->index[i]; - /* - * Allocate new chunk with size reduced by 1 slot. - * Copy all indexes to that chunk except for one - * to be deleted. - */ - if (idx->def->iid == iid) { - index_def_delete(idx->def); - free(idx); - size_t idx_sz = sizeof(struct index *); - uint32_t idx_count = space->index_count - 1; - struct index **new_indexes = - (struct index **) malloc(idx_sz * idx_count); - if (new_indexes == NULL) { - diag_set(OutOfMemory, idx_sz * idx_count, - "malloc", "new_indexes"); - return; - } - memcpy(new_indexes, space->index, i * idx_sz); - memcpy(new_indexes + i, space->index + i + 1, - idx_sz * (idx_count - i)); - free(space->index); - space->index = new_indexes; - space->index_count--; - break; - } + if (space->index[i]->def->iid == iid) + to_delete = space->index[i]; + else if (new_index_id_max < space->index[i]->def->iid) + new_index_id_max = space->index[i]->def->iid; } + /* + * Allocate new chunk with size reduced by 1 slot. Copy + * all indexes to that chunk except for one to be deleted. + */ + assert(to_delete != NULL); + uint32_t new_index_count = space->index_count - 1; + uint32_t size = (new_index_count + new_index_id_max + 1) * + sizeof(struct index *); + struct index **new_index_map = (struct index **) malloc(size); + if (new_index_map == NULL) { + diag_set(OutOfMemory, size, "malloc", "new_index_map"); + return -1; + } + /* Nullify gaps. */ + memset(new_index_map, 0, + sizeof(struct index *) * (new_index_id_max + 1)); + struct index **new_indexes = new_index_map + new_index_id_max + 1; + for (uint32_t i = 0, j = 0; i < space->index_count; ++i) { + struct index *idx = space->index[i]; + if (idx == to_delete) + continue; + new_index_map[idx->def->iid] = space->index_map[idx->def->iid]; + new_indexes[j++] = idx; + } + index_def_delete(to_delete->def); + free(to_delete); + free(space->index_map); + space->index_map = new_index_map; + space->index = new_indexes; + space->index_count--; + space->index_id_max = new_index_id_max; struct session *user_session = current_session(); user_session->sql_flags |= SQLITE_InternChanges; + return 0; } /* @@ -266,16 +278,11 @@ table_delete(struct sqlite3 *db, struct Table *tab) goto skip_index_delete; /* Delete all indices associated with this table. */ for (uint32_t i = 0; i < tab->space->index_count; ++i) { - /* - * These indexes are just wrapper for - * index_def's, so it makes no sense to call - * index_delete(). - */ struct index *idx = tab->space->index[i]; - free(idx->def); + index_def_delete(idx->def); free(idx); } - free(tab->space->index); + free(tab->space->index_map); free(tab->space); skip_index_delete: assert(tab->def != NULL); @@ -362,14 +369,6 @@ sqlite3CheckIdentifierName(Parse *pParse, char *zName) return SQLITE_OK; } -struct index * -sql_table_primary_key(const struct Table *tab) -{ - if (tab->space->index_count == 0 || tab->space->index[0]->def->iid != 0) - return NULL; - return tab->space->index[0]; -} - /** * Create and initialize a new SQL Table object. * All memory except table object itself is allocated on region. @@ -812,7 +811,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ int nTerm; if (pTab == 0) goto primary_key_exit; - if (sql_table_primary_key(pTab) != NULL) { + if (space_index(pTab->space, 0) != NULL) { sqlite3ErrorMsg(pParse, "table \"%s\" has more than one primary key", pTab->def->name); @@ -872,7 +871,7 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ goto primary_key_exit; } - struct index *pk = sql_table_primary_key(pTab); + struct index *pk = space_index(pTab->space, 0); assert(pk != NULL); struct key_def *pk_key_def = pk->def->key_def; for (uint32_t i = 0; i < pk_key_def->part_count; i++) { @@ -1146,11 +1145,13 @@ getNewSpaceId(Parse * pParse) * @param idx_def Definition of index under construction. * @param pk_def Definition of primary key index. * @param space_id_reg Register containing generated space id. + * @param index_id_reg Register containing generated index id. */ static void vdbe_emit_create_index(struct Parse *parse, struct space_def *def, const struct index_def *idx_def, - const struct index_def *pk_def, int space_id_reg) + const struct index_def *pk_def, int space_id_reg, + int index_id_reg) { struct Vdbe *v = sqlite3GetVdbe(parse); int entry_reg = ++parse->nMem; @@ -1187,10 +1188,11 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def, } else { /* * An existing table is being modified; - * space_id_reg is register, but iid is literal. + * space_id_reg is literal, index_id_reg is + * register. */ sqlite3VdbeAddOp2(v, OP_Integer, space_id_reg, entry_reg); - sqlite3VdbeAddOp2(v, OP_SCopy, idx_def->iid, entry_reg + 1); + sqlite3VdbeAddOp2(v, OP_SCopy, index_id_reg, entry_reg + 1); } sqlite3VdbeAddOp4(v, OP_String8, 0, entry_reg + 2, 0, sqlite3DbStrDup(parse->db, idx_def->name), @@ -1612,7 +1614,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } if (!p->def->opts.is_view) { - if (sql_table_primary_key(p) == NULL) { + if (space_index(p->space, 0) == NULL) { sqlite3ErrorMsg(pParse, "PRIMARY KEY missing on table %s", p->def->name); @@ -1686,12 +1688,12 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ createSpace(pParse, reg_space_id, stmt); /* Indexes aren't required for VIEW's.. */ if (!p->def->opts.is_view) { - struct index *pk = sql_table_primary_key(p); + struct index *pk = space_index(p->space, 0); for (uint32_t i = 0; i < p->space->index_count; ++i) { struct index *idx = p->space->index[i]; - idx->def->iid = i; vdbe_emit_create_index(pParse, p->def, idx->def, - pk->def, reg_space_id); + pk->def, reg_space_id, + idx->def->iid); } } @@ -1739,7 +1741,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } fk->parent_id = reg_space_id; } else if (fk_parse->is_self_referenced) { - struct index *pk = sql_table_primary_key(p); + struct index *pk = space_index(p->space, 0); if (pk->def->key_def->part_count != fk->field_count) { diag_set(ClientError, ER_CREATE_FK_CONSTRAINT, fk->name, "number of columns in "\ @@ -2480,30 +2482,39 @@ getNewIid(Parse * pParse, int iSpaceId, int iCursor) /** * Add new index to table's indexes list. - * We follow convention that PK comes first in list. - * - * @param index Index to be added to list. * @param tab Table to which belongs given index. + * @param index Index to be added to list. */ static void table_add_index(struct Table *tab, struct index *index) { - uint32_t idx_count = tab->space->index_count; - size_t indexes_sz = sizeof(struct index *) * (idx_count + 1); - struct index **idx = (struct index **) realloc(tab->space->index, - indexes_sz); - if (idx == NULL) { - diag_set(OutOfMemory, indexes_sz, "realloc", "idx"); + struct space *space = tab->space; + uint32_t new_index_count = space->index_count + 1; + uint32_t new_index_id_max = MAX(space->index_id_max, index->def->iid); + uint32_t size = + (new_index_id_max + 1 + new_index_count) * sizeof(index); + struct index **new_index_map = (struct index **) malloc(size); + if (new_index_map == NULL) { + diag_set(OutOfMemory, size, "malloc", "new_index_map"); return; } - tab->space->index = idx; - /* Make sure that PK always comes as first member. */ - if (index->def->iid == 0 && idx_count != 0) { - struct index *tmp = tab->space->index[0]; - tab->space->index[0] = index; - index = tmp; - } - tab->space->index[tab->space->index_count++] = index; + struct index **new_index = new_index_map + new_index_id_max + 1; + memcpy(new_index_map, space->index_map, + sizeof(index) * (space->index_id_max + 1)); + memcpy(new_index, space->index, sizeof(index) * space->index_count); + new_index_map[index->def->iid] = index; + /* + * Make sure that PK always comes as first member in order + * to prefer it among other indexes when possible. + */ + if (index->def->iid == 0 && space->index_count > 0) + SWAP(index, new_index[0]); + new_index[new_index_count - 1] = index; + free(space->index_map); + space->index_map = new_index_map; + space->index = new_index; + space->index_count = new_index_count; + space->index_id_max = new_index_id_max; } /** @@ -2798,7 +2809,11 @@ sql_create_index(struct Parse *parse, struct Token *token, * (for the simplicity sake we set it to 1), but PK * still must have iid == 0. */ - uint32_t iid = idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK; + uint32_t iid; + if (idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK) + iid = table->space->index_id_max + 1; + else + iid = 0; if (index_fill_def(parse, index, table, iid, name, strlen(name), col_list, idx_type, sql_stmt) != 0) goto exit_create_index; @@ -2858,6 +2873,7 @@ sql_create_index(struct Parse *parse, struct Token *token, if (table == parse->pNewTable) { for (uint32_t i = 0; i < table->space->index_count; ++i) { struct index *existing_idx = table->space->index[i]; + uint32_t iid = existing_idx->def->iid; struct key_def *key_def = index->def->key_def; struct key_def *exst_key_def = existing_idx->def->key_def; @@ -2882,7 +2898,9 @@ sql_create_index(struct Parse *parse, struct Token *token, constraint_is_named(existing_idx->def->name); /* CREATE TABLE t(a, UNIQUE(a), PRIMARY KEY(a)). */ if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK && - existing_idx->def->iid != 0 && !is_named) { + iid != 0 && !is_named) { + table->space->index_map[0] = existing_idx; + table->space->index_map[iid] = NULL; existing_idx->def->iid = 0; goto exit_create_index; } @@ -2938,9 +2956,9 @@ sql_create_index(struct Parse *parse, struct Token *token, space_id = table->def->id; index_id = getNewIid(parse, space_id, cursor); sqlite3VdbeAddOp1(vdbe, OP_Close, cursor); - struct index *pk = sql_table_primary_key(table); + struct index *pk = space_index(table->space, 0); vdbe_emit_create_index(parse, table->def, index->def, pk->def, - space_id); + space_id, index_id); /* Consumes sql_stmt. */ first_schema_col = vdbe_emit_index_schema_record(parse, index->def->name, diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c index 0f285cc8b..8ec198d88 100644 --- a/src/box/sql/delete.c +++ b/src/box/sql/delete.c @@ -351,7 +351,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, * key. */ key_len = 0; - struct index *pk = sql_table_primary_key(table); + struct index *pk = space_index(space, 0); const char *zAff = is_view ? NULL : sql_space_index_affinity_str(parse->db, space->def, diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index dcb57e9b3..f26372ce7 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3140,7 +3140,7 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ struct Table *tab = src_list->a[0].pTab; assert(tab != NULL); - struct index *pk = sql_table_primary_key(tab); + struct index *pk = space_index(tab->space, 0); assert(pk != NULL); uint32_t fieldno = pk->def->key_def->parts[0].fieldno; diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 9a7fdffbb..cde3bc34c 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -960,7 +960,7 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct Table *tab, * FIXME: should be removed after introducing * strict typing. */ - struct index *pk = sql_table_primary_key(tab); + struct index *pk = space_index(tab->space, 0); uint32_t part_count = pk->def->key_def->part_count; if (part_count == 1) { uint32_t fieldno = pk->def->key_def->parts[0].fieldno; diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 3ba7ad022..c5b664414 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -443,7 +443,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ break; struct space *space = space_cache_find(table->def->id); struct space_def *def = space->def; - struct index *pk = sql_table_primary_key(table); + struct index *pk = space_index(space, 0); pParse->nMem = 6; if (def->opts.is_view) { const char *sql = table->def->opts.sql; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 02732b59c..9f1791a6e 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3343,12 +3343,6 @@ int sqlite3ColumnsFromExprList(Parse *parse, ExprList *expr_list, Table *table); void sqlite3SelectAddColumnTypeAndCollation(Parse *, Table *, Select *); Table *sqlite3ResultSetOfSelect(Parse *, Select *); -/** - * Return the PRIMARY KEY index of a table. - */ -struct index * -sql_table_primary_key(const struct Table *tab); - void sqlite3StartTable(Parse *, Token *, int); void sqlite3AddColumn(Parse *, Token *, Token *); @@ -3667,8 +3661,11 @@ void sqlite3UnlinkAndDeleteTable(sqlite3 *, const char *); * * @param space Space which index belongs to. * @param iid Id of index to be deleted. + * + * @retval 0 Success. + * @retval -1 Error. */ -void +int sql_space_index_delete(struct space *space, uint32_t iid); char *sqlite3NameFromToken(sqlite3 *, Token *); diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h index f3e65303b..d2475bbc8 100644 --- a/src/box/sql/tarantoolInt.h +++ b/src/box/sql/tarantoolInt.h @@ -207,7 +207,6 @@ fkey_encode_links(struct region *region, const struct fkey_def *def, int type, uint32_t *size); /** -<<<<<<< HEAD * Encode index parts of given foreign key constraint into * MsgPack on @region. * @param region Region to use. diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 076b28a94..852be6fba 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -146,7 +146,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ /* Allocate cursor on primary index. */ int pk_cursor = pParse->nTab++; pTabList->a[0].iCursor = pk_cursor; - struct index *pPk = sql_table_primary_key(pTab); + struct index *pPk = space_index(pTab->space, 0); i = sizeof(int) * def->field_count; aXRef = (int *) region_alloc(&pParse->region, i); if (aXRef == NULL) { diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index b956726e4..21136688c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4746,7 +4746,10 @@ case OP_DropTable: { * This is called after an index is dropped from Tarantool DD. */ case OP_DropIndex: { - sql_space_index_delete(pOp->p4.space, pOp->p1); + if (sql_space_index_delete(pOp->p4.space, pOp->p1) != 0) { + rc = SQL_TARANTOOL_ERROR; + goto abort_due_to_error; + } break; } diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 3e50f9b89..ef70442b3 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -1089,7 +1089,7 @@ valueNew(sqlite3 * db, struct ValueNewStat4Ctx *p) uint32_t part_count = idx->key_def->part_count; int nByte = sizeof(Mem) * part_count + - ROUND8(sizeof(UnpackedRecord)); + ROUND8(sizeof(UnpackedRecord)); pRec = (UnpackedRecord *) sqlite3DbMallocZero(db, nByte); if (pRec == NULL) diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index 1aa858ac1..e2aaca3c3 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -1351,7 +1351,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t Expr *pAndExpr = 0; /* An ".. AND (...)" expression */ Table *pTab = pTabItem->pTab; struct key_def *pk_key_def = - sql_table_primary_key(pTab)->def->key_def; + space_index(pTab->space, 0)->def->key_def; uint32_t pk_part_count = pk_key_def->part_count; pTerm = pLoop->aLTerm[0]; diff --git a/test/sql-tap/gh2130-index-refer-table.test.lua b/test/sql-tap/gh2130-index-refer-table.test.lua index 3a6064cb9..b5fc1106d 100755 --- a/test/sql-tap/gh2130-index-refer-table.test.lua +++ b/test/sql-tap/gh2130-index-refer-table.test.lua @@ -69,16 +69,4 @@ test:do_execsql_test( -- }) --- This part of test is banned in scope of #2174 --- test:do_execsql_test( --- "index-1.6", --- [[ --- REINDEX t1ix1 ON t1; --- ]], --- { --- -- --- --- -- --- }) - test:finish_test() diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua index fdce2683c..90ce4b232 100755 --- a/test/sql-tap/index1.test.lua +++ b/test/sql-tap/index1.test.lua @@ -119,67 +119,6 @@ test:do_test( -- }) --- MUST_WORK_TEST REINDEX and integrity_check -if (0 > 0) - then - -- Try creating a bunch of indices on the same table - -- - local r = {} - for i = 1, 99, 1 do - table.insert(r,string.format("index%02d", i)) - end - test:do_test( - "index-3.1", - function() - test:execsql("CREATE TABLE test1(f1 int primary key, f2 int, f3 int, f4 int, f5 int)") - for i = 1, 99, 1 do - local sql = string.format("CREATE INDEX %s ON test1(f%s)", string.format("index%02d", i), (i%5)+1) - test:execsql(sql) - end - return test:execsql [[SELECT name FROM sqlite_master - WHERE type='index' AND tbl_name='test1' - ORDER BY name]] - end, { - -- - r - -- - }) - - X(104, "X!cmd", [=[["integrity_check","index-3.2.1"]]=]) - test:do_execsql_test( - "index-3.2.2", - [[ - REINDEX - ]], { - -- - - -- - }) - - - - --X(110, "X!cmd", [=[["integrity_check","index-3.2.3"]]=]) - -- Verify that all the indices go away when we drop the table. - -- - test:do_test( - "index-3.3", - function() - test:execsql "DROP TABLE test1" - return test:execsql [[SELECT name FROM sqlite_master - WHERE type='index' AND tbl_name='test1' - ORDER BY name]] - end, { - -- - - -- - }) - - -- Create a table and insert values into that table. Then create - -- an index on that table. Verify that we can select values - -- from the table correctly using the index - -- Note that the index names index9 and indext are chosen because - -- they both have the same hash. -end test:do_test( "index-4.1", function() @@ -1017,7 +956,7 @@ test:do_execsql_test( SELECT "_index"."name" FROM "_index" JOIN "_space" WHERE "_index"."id" = "_space"."id" AND "_space"."name"='T7'; ]], { -- - "pk_unnamed_T7_3", "unique_unnamed_T7_2", "unique_unnamed_T7_1" + "pk_unnamed_T7_3", "unique_unnamed_T7_1", "unique_unnamed_T7_2" -- }) diff --git a/test/sql-tap/index6.test.lua b/test/sql-tap/index6.test.lua index 069623f66..af18e89d4 100755 --- a/test/sql-tap/index6.test.lua +++ b/test/sql-tap/index6.test.lua @@ -106,13 +106,6 @@ test:plan(14) -- SELECT idx, stat FROM sqlite_stat1 ORDER BY idx; -- } -- } {{} 15 t1a {10 1} t1b {8 1} ok} --- do_test index6-1.14 { --- execsql { --- REINDEX; --- ANALYZE; --- SELECT idx, stat FROM sqlite_stat1 ORDER BY idx; --- } --- } {{} 15 t1a {10 1} t1b {8 1} ok} -- do_test index6-1.15 { -- execsql { -- CREATE INDEX t1c ON t1(c); diff --git a/test/sql-tap/index7.test.lua b/test/sql-tap/index7.test.lua index 4f9070813..7d4a54723 100755 --- a/test/sql-tap/index7.test.lua +++ b/test/sql-tap/index7.test.lua @@ -126,14 +126,6 @@ end -- PRAGMA integrity_check; -- } -- } {t1 {15 1} t1a {10 1} t1b {8 1} ok} --- do_test index7-1.14 { --- execsql { --- REINDEX; --- ANALYZE; --- SELECT idx, stat FROM sqlite_stat1 ORDER BY idx; --- PRAGMA integrity_check; --- } --- } {t1 {15 1} t1a {10 1} t1b {8 1} ok} -- do_test index7-1.15 { -- execsql { -- CREATE INDEX t1c ON t1(c); diff --git a/test/sql-tap/insert1.test.lua b/test/sql-tap/insert1.test.lua index 750732d37..cfca0025f 100755 --- a/test/sql-tap/insert1.test.lua +++ b/test/sql-tap/insert1.test.lua @@ -209,11 +209,6 @@ end, { -- do_test insert-3.4 { -- execsql {SELECT * FROM test2 WHERE f1=22 AND f2=-4.44} -- } {22 -4.44 hi abc-123 wham} --- ifcapable {reindex} { --- do_test insert-3.5 { --- execsql REINDEX --- } {} --- } -- integrity_check insert-3.5 -- Test of expressions in the VALUES clause -- @@ -395,11 +390,6 @@ test:do_execsql_test("insert-4.7", [[ -- SELECT * FROM t1 WHERE b=3; -- } -- } {} - -- ifcapable {reindex} { - -- do_test insert-6.5 { - -- execsql REINDEX - -- } {} - -- } -- do_test insert-6.6 { -- execsql { -- DROP TABLE t1; diff --git a/test/sql-tap/misc3.test.lua b/test/sql-tap/misc3.test.lua index 92f8210c9..dc1545f8f 100755 --- a/test/sql-tap/misc3.test.lua +++ b/test/sql-tap/misc3.test.lua @@ -443,46 +443,6 @@ test:do_test( -- }) --- Do some additional EXPLAIN operations to exercise the displayP4 logic. --- This part of test is disabled in scope of #2174 --- test:do_test( --- "misc3-6.10", --- function() --- local x = test:execsql([[ --- CREATE TABLE ex1( --- id PRIMARY KEY, --- a INTEGER DEFAULT 54321, --- b TEXT DEFAULT "hello", --- c REAL DEFAULT 3.1415926 --- ); --- CREATE UNIQUE INDEX ex1i1 ON ex1(a); --- EXPLAIN REINDEX; --- ]]) --- x = json.encode(x) --- return string.find(x, "\"SorterCompare\",%d+,%d+,%d+") > 0 --- end, true) --- --- test:do_test( --- "misc3-6.11-utf8", --- function() --- local x = test:execsql([[ --- EXPLAIN SELECT a+123456789012, b*4.5678, c FROM ex1 ORDER BY +a, b DESC --- ]]) --- x = json.encode(x) --- local y = {} --- table.insert(y, string.find(x, "123456789012")>0) --- table.insert(y, string.find(x, "4.5678")>0) --- table.insert(y, string.find(x, "hello")>0) --- table.insert(y, string.find(x, "-B")>0) --- return y --- end, { --- -- --- 1, 1, 1, 1 --- -- --- }) - - - -- MUST_WORK_TEST autoincrement for pk if (0 > 0) then -- Ticket #640: vdbe stack overflow with a LIMIT clause on a SELECT inside diff --git a/test/sql-tap/reindex.test.lua b/test/sql-tap/reindex.test.lua deleted file mode 100755 index d4f8c8188..000000000 --- a/test/sql-tap/reindex.test.lua +++ /dev/null @@ -1,36 +0,0 @@ -#!/usr/bin/env tarantool -test = require("sqltester") -test:plan(3) - - -test:execsql("CREATE TABLE t1(a INT PRIMARY KEY);") -test:execsql("CREATE INDEX t1ix1 ON t1(a)") - -test:do_execsql_test( - "reindex-1.1", - "REINDEX t1ix1 ON t1", - { - -- - - -- - }) - -test:do_catchsql_test( - "reindex-1.2", - "REINDEX t1ix2 ON t1", - { - -- - 1, "unable to identify the object to be reindexed" - -- - }) - -test:do_catchsql_test( - "reindex-1.3", - "REINDEX t1ix1 ON t3", - { - -- - 1, "no such table: T3" - -- - }) - -test:finish_test() diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini index 0637cffc1..2455f5e8d 100644 --- a/test/sql-tap/suite.ini +++ b/test/sql-tap/suite.ini @@ -1,8 +1,6 @@ [default] core = app description = Database tests with #! using TAP -disabled = - reindex.test.lua ; This test is banned in scope of #2174 lua_libs = lua/sqltester.lua ../sql/lua/sql_tokenizer.lua ../box/lua/identifier.lua is_parallel = True release_disabled = debug_mode_only.test.lua