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

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Thu Jun 28 21:49:38 MSK 2018


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 at 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;




More information about the Tarantool-patches mailing list