[tarantool-patches] Re: [PATCH v6] sql: add index_def to struct Index
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jun 29 23:46:28 MSK 2018
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.
More information about the Tarantool-patches
mailing list