[tarantool-patches] Re: [PATCH v6] sql: add index_def to struct Index

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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.