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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <ivan.koptelov@tarantool.org>
> 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 "<name> PRIMARY KEY COLLATE <type>",
>  		 * 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;