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

["n.pettik" <korablev@tarantool.org>]

Firstly, I have found some bugs:

tarantool> create table t1(a,b,c,d, primary key(a,a,a,b,c));
---
...

It is OK, but when I do this:

tarantool> create index i1 on t1(b,c,a,c)
---
...

tarantool> create index i1 on t1(b,c,a,c)
---
...

tarantool> create index i1 on t1(b,c,a,c)
---
…

No error is raised and index isn’t created.

After you find and fix this bug, add also test on this case.

Then:

CREATE TABLE t11 (s1 INT, a, constraint c1 UNIQUE(s1) on conflict replace, PRIMARY KEY(s1));

In this case creation of unique constraint c1 is omitted, but no errors or warnings are shown.
It is not a problem now, but when ALTER TABLE DROP CONSTRAINT is implemented,
it will be possible to drop c1 constraint. Eventually, user would be disappointed if tried to drop
this constraint but got an error.

> @@ -1022,19 +1019,18 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
> 		 */
> 		assert(regKey == (regStat4 + 2));
> 		Index *pPk = sqlite3PrimaryKeyIndex(pIdx->pTable);
> -		int j, k, regKeyStat;
> -		int nPkColumn = (int)index_column_count(pPk);
> -		regKeyStat = sqlite3GetTempRange(pParse, nPkColumn);
> -		for (j = 0; j < nPkColumn; j++) {
> -			k = pPk->aiColumn[j];
> -			assert(k >= 0 && k < (int)pTab->def->field_count);
> -			sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, k, regKeyStat + j);
> -			VdbeComment((v, "%s",
> -				pTab->def->fields[pPk->aiColumn[j]].name));
> +		int pk_part_count = (int) pPk->def->key_def->part_count;
> +		int regKeyStat = sqlite3GetTempRange(pParse, pk_part_count);
> +		for (int j = 0; j < pk_part_count; ++j) {

Use uint32_t instead of int:

-               int pk_part_count = (int) pPk->def->key_def->part_count;
+               uint32_t pk_part_count = pPk->def->key_def->part_count;
                int regKeyStat = sqlite3GetTempRange(pParse, pk_part_count);
-               for (int j = 0; j < pk_part_count; ++j) {
-                       int k = pPk->def->key_def->parts[j].fieldno;
-                       assert(k >= 0 && k < (int) pTab->def->field_count);
+               for (uint32_t j = 0; j < pk_part_count; ++j) {
+                       uint32_t k = pPk->def->key_def->parts[j].fieldno;
+                       assert(k >= 0 && k < pTab->def->field_count);

> +struct Index *
> +sql_index_alloc(struct sqlite3 *db, uint32_t part_count)
> {
> -	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);
> -	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;
> -		p->sort_order = (enum sort_order *) pExtra;
> -		p->nColumn = nCol;
> -		*ppExtra = ((char *)p) + nByte;
> -	}
> +	/* Size of struct Index and aiRowLogEst. */
> +	int nByte = ROUND8(sizeof(struct Index)) +
> +		    ROUND8(sizeof(LogEst) * (part_count + 1));

Do we really need this alignment?

> +	struct Index *p = sqlite3DbMallocZero(db, nByte);
> +	if (p != NULL)
> +		p->aiRowLogEst = (LogEst *) ((char *)p + ROUND8(sizeof(*p)));
> 	return p;
> }
> @@ -2631,46 +2520,187 @@ addIndexToTable(Index * pIndex, Table * pTab)
> +/**
> + * Allocate memory on parser region and copy given string (part of
> + * the sql statement) into the allocated memory.
> + * @param parse Parse context.
> + * @param str String (a part of sql statement) to be copied.
> + *
> + * @retval size Appended size.
> + */
> +static int
> +sql_append(struct Parse *parse, const char *str)

Such strange function...Lets rename it to sql(or str)_copy_to_region() at least.

> +{
> +	const size_t str_len = strlen(str);
> +	char *str_part = region_alloc(&parse->region, str_len);
> +	if (str_part == NULL) {
> +		diag_set(OutOfMemory, str_len, "region_alloc", "str_part");
> +		parse->rc = SQL_TARANTOOL_ERROR;
> +		parse->nErr++;
> +		return 0;
> +	}
> +	memcpy(str_part, str, str_len);
> +	return str_len;
> +}
> +
> +/**
> + * Create and set index_def in the given Index.
> + *
> + * @param parse Parse context.
> + * @param index Index for which index_def should be created. It is
> + *              used only to set index_def at the end of the
> + *              function.
> + * @param table Table which is indexed by 'index' param.
> + * @param iid Index ID.
> + * @param name Index name.
> + * @param name_len Index name length.
> + * @param is_unique Is given 'index' unique or not.
> + * @param expr_list List of expressions, describe which columns
> + *                  of 'table' are used in index and also their
> + *                  collations, orders, etc.
> + * @param idx_type Index type, one of the following:
> + *                 SQLITE_IDXTYPE_APPDEF 0 (Index is created with
> + *                 CREATE INDEX statement)
> + *                 SQLITE_IDXTYPE_UNIQUE 1 (Index is created
> + *                 automatically to implement a UNIQUE constraint)
> + *                 SQLITE_IDXTYPE_PRIMARYKEY 2 (Index is a PRIMARY
> + *                 KEY).

Yo can skip this description - it is almost copy of existing one at these macroses definition.

> + */
> +static void
> +set_index_def(struct Parse *parse, struct Index *index, struct Table *table,

Lets use Tarantool naming convention, sort of: index_fill_def() or index_create_def().

> +	      uint32_t iid, const char *name, uint32_t name_len, bool is_unique,
> +	      struct ExprList *expr_list, u8 idx_type)
> +{
> +	struct space_def *space_def = table->def;
> +	size_t sql_size = 0;
> +	struct index_opts opts;
> +	index_opts_create(&opts);
> +	index->def = NULL;
> +	opts.is_unique = is_unique;
> +	int rc = -1;

You don’t use this variable and in the end just reassign it:

+	if (index->def == NULL)
+		goto tnt_error;
+	rc = 0;
+cleanup:
+	if (key_def != NULL)
+		key_def_delete(key_def);
+	return rc;

> +
> +	struct key_def *key_def = key_def_new(expr_list->nExpr);
> +	if (key_def == NULL)
> +		goto tnt_error;
> +
> +	/* Build initial parts of SQL statement.  */
> +	if (idx_type == SQLITE_IDXTYPE_APPDEF) {
> +		sql_size += sql_append(parse, "CREATE INDEX ");
> +		sql_size += sql_append(parse, name);
> +		sql_size += sql_append(parse, " ON ");
> +		sql_size += sql_append(parse, space_def->name);
> +		sql_size += sql_append(parse, " (“);

Why do you need to construct “CREATE INDEX” statement from scratch?
This function is only called from sql_create_index() - there you already have
this string:

/*
 * Gather the complete text of the CREATE INDEX
 * statement into the zStmt variable
 */
assert(start != NULL);
int n = (int)(parse->sLastToken.z - token->z) +
       parse->sLastToken.n;
if (token->z[n - 1] == ';')
       n--;

>  void
> sql_create_index(struct Parse *parse, struct Token *token,
> 		 struct SrcList *tbl_name, struct ExprList *col_list,
> -		 int on_error, struct Token *start, struct Expr *where,
> -		 enum sort_order sort_order, bool if_not_exist, u8 idx_type)
> -{
> -	Table *pTab = 0;	/* Table to be indexed */
> -	Index *pIndex = 0;	/* The index to be created */
> -	char *zName = 0;	/* Name of the index */
> -	int nName;		/* Number of characters in zName */
> -	int i, j;
> -	DbFixer sFix;		/* For assigning database names to pTable */
> -	sqlite3 *db = parse->db;
> -	struct ExprList_item *col_listItem;	/* For looping over col_list */
> -	int nExtra = 0;		/* Space allocated for zExtra[] */
> -	char *zExtra = 0;	/* Extra space after the Index object */
> +		 enum on_conflict_action on_error, struct Token *start,
> +		 struct Expr *where, enum sort_order sort_order,
> +		 bool if_not_exist, u8 idx_type)
> +{
> +	/* Table to be indexed.  */

Extra space after dot.

> +	struct Table *table = NULL;
> +	/* The index to be created.  */
> +	struct Index *index = NULL;
> +	/* Name of the index.  */
> +	char *name = NULL;
> +	int name_len;

You don’t need to declare variables so beforehand.
Its first usage occurs at 130+ lines below.

> +	struct sqlite3 *db = parse->db;
> 	struct session *user_session = current_session();
> -	if (db->mallocFailed || parse->nErr > 0) {
> +	if (db->mallocFailed || parse->nErr > 0)
> 		goto exit_create_index;
> -	}
> -	/* Do not account nested operations: the count of such
> -	 * operations depends on Tarantool data dictionary internals,
> -	 * such as data layout in system spaces. Also do not account
> -	 * PRIMARY KEY and UNIQUE constraint - they had been accounted
> -	 * in CREATE TABLE already.
> +	/*
> +	 * Do not account nested operations: the count of such
> +	 * operations depends on Tarantool data dictionary
> +	 * internals, such as data layout in system spaces. Also
> +	 * do not account PRIMARY KEY and UNIQUE constraint - they
> +	 * had been accounted in CREATE TABLE already.
> 	 */
> 	if (!parse->nested && idx_type == SQLITE_IDXTYPE_APPDEF) {
> 		Vdbe *v = sqlite3GetVdbe(parse);
> @@ -2681,39 +2711,43 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 	assert(db->pSchema != NULL);
>  	/*
> -	 * Find the table that is to be indexed.  Return early if not found.
> +	 * Find the table that is to be indexed.
> +	 * Return early if not found.
> 	 */
> 	if (tbl_name != NULL) {
> -
> -		/* Use the two-part index name to determine the database
> -		 * to search for the table. 'Fix' the table name to this db
> -		 * before looking up the table.
> +		/*
> +		 * Use the two-part index name to determine the
> +		 * database to search for the table. 'Fix' the
> +		 * table name to this db before looking up the
> +		 * table.
> 		 */
> 		assert(token && token->z);
> -
> -		sqlite3FixInit(&sFix, parse, "index", token);
> -		if (sqlite3FixSrcList(&sFix, tbl_name)) {
> -			/* Because the parser constructs tbl_name from a single identifier,
> +		DbFixer db_fixer;
> +		sqlite3FixInit(&db_fixer, parse, "index", token);
> +		if (sqlite3FixSrcList(&db_fixer, tbl_name)) {

This ‘Fix’ routine seems to be useless now, lets remove it.

> +			/*
> +			 * Because the parser constructs tbl_name
> +			 * from a single identifier,
> 			 * sqlite3FixSrcList can never fail.
> 			 */
> -			assert(0);
> +			unreachable();
> 		}
> -		pTab = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
> -		assert(db->mallocFailed == 0 || pTab == 0);
> -		if (pTab == 0)
> +		table = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
> +		assert(db->mallocFailed == 0 || table == NULL);
> +		if (table == NULL)
> 			goto exit_create_index;
> -		sqlite3PrimaryKeyIndex(pTab);
> +		sqlite3PrimaryKeyIndex(table);

Why do you call this function? It returns PK, but you don’t use it.

> 	} else {
> 		assert(token == NULL);
> 		assert(start == NULL);
> -		pTab = parse->pNewTable;
> -		if (!pTab)
> +		table = parse->pNewTable;
> +		if (table == NULL)
> 			goto exit_create_index;
> 	}

Instead of checking table on NULL in each branch and after that using assert(table != NULL),
it is better to replace that assert() with check:

-               if (table == NULL)
-                       goto exit_create_index;
-               sqlite3PrimaryKeyIndex(table);
        } else {
                assert(token == NULL);
                assert(start == NULL);
                table = parse->pNewTable;
-               if (table == NULL)
-                       goto exit_create_index;
        }
 
-       assert(table != NULL);
+       if (table == NULL)
+               goto exit_create_index;

> -	assert(pTab != 0);
> +	assert(table != NULL);
> 	assert(parse->nErr == 0);
> -	if (pTab->def->opts.is_view) {
> +	if (table->def->opts.is_view) {
> 		sqlite3ErrorMsg(parse, "views may not be indexed");
> 		goto exit_create_index;
> 	}

Lets also prohibit creation of indexes on system spaces.

> @@ -2731,42 +2765,38 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 	 * primary key or UNIQUE constraint.  We have to invent
> 	 * our own name.
> 	 */
> -	if (token) {
> -		zName = sqlite3NameFromToken(db, token);
> -		if (zName == 0)
> +	if (token != NULL) {
> +		name = sqlite3NameFromToken(db, token);
> +		if (name == NULL)
> 			goto exit_create_index;
> -		assert(token->z != 0);
> +		assert(token->z != NULL);
> 		if (!db->init.busy) {
> -			if (sqlite3HashFind(&db->pSchema->tblHash, zName) !=
> +			if (sqlite3HashFind(&db->pSchema->tblHash, name) !=
> 			    NULL) {
> -				sqlite3ErrorMsg(parse,
> -						"there is already a table named %s",
> -						zName);
> +				sqlite3ErrorMsg(parse, "there is already a "\
> +						"table named %s", name);
> 				goto exit_create_index;
> 			}
> 		}
> -		if (sqlite3HashFind(&pTab->idxHash, zName) != NULL) {
> +		if (sqlite3HashFind(&table->idxHash, name) != NULL) {
> 			if (!if_not_exist) {
> 				sqlite3ErrorMsg(parse,
> 						"index %s.%s already exists",
> -						pTab->def->name, zName);
> +						table->def->name, name);
> 			} else {
> 				assert(!db->init.busy);
> 			}
> 			goto exit_create_index;
> 		}
> 	} else {
> -		int n;
> -		Index *pLoop;
> -		for (pLoop = pTab->pIndex, n = 1; pLoop;
> +		int n = 1;
> +		for (struct Index *pLoop = table->pIndex; pLoop != NULL;

Lets use Tarantool naming convention.

> 		     pLoop = pLoop->pNext, n++) {
> 		}
> -		zName =
> -		    sqlite3MPrintf(db, "sqlite_autoindex_%s_%d", pTab->def->name,
> -				   n);
> -		if (zName == 0) {
> +		name = sqlite3MPrintf(db, "sqlite_autoindex_%s_%d”,

Lets remove ’sqlite_’ prefix and use just ’sql_’.

> +				      table->def->name, n);
> +		if (name == NULL)
> 			goto exit_create_index;
> -		}
> 	}
>  	/*
> @@ -2776,9 +2806,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 	 * simulate this.
> 	 */
> 	if (col_list == NULL) {
> -		Token prevCol;
> -		uint32_t last_field = pTab->def->field_count - 1;
> -		sqlite3TokenInit(&prevCol, pTab->def->fields[last_field].name);
> +		struct Token prevCol;

Lets use Tarantool naming convention.

> +		uint32_t last_field = table->def->field_count - 1;
> +		sqlite3TokenInit(&prevCol, table->def->fields[last_field].name);
> 		col_list = sql_expr_list_append(parse->db, NULL,
> 						sqlite3ExprAlloc(db, TK_ID,
> 								 &prevCol, 0));
> @@ -2790,108 +2820,93 @@ sql_create_index(struct Parse *parse, struct Token *token,
> 		sqlite3ExprListCheckLength(parse, col_list, "index");
> 	}
> -	/* Figure out how many bytes of space are required to store explicitly
> -	 * specified collation sequence names.
> -	 */
> -	for (i = 0; i < col_list->nExpr; i++) {
> -		Expr *pExpr = col_list->a[i].pExpr;
> -		assert(pExpr != 0);
> -		if (pExpr->op == TK_COLLATE) {
> -			nExtra += (1 + sqlite3Strlen30(pExpr->u.zToken));
> -		}
> -	}
> +	/* Allocate the index structure.  */

Extra space after dot.

> +	name_len = sqlite3Strlen30(name);

Lets use traditional strlen() instead of SQLite analogs.

> -	/*
> -	 * Allocate the index structure.
> -	 */
> -	nName = sqlite3Strlen30(zName);
> -	pIndex = sqlite3AllocateIndexObject(db, col_list->nExpr,
> -					    nName + nExtra + 1, &zExtra);
> -	if (db->mallocFailed) {
> +	if (name_len > BOX_NAME_MAX) {
> +		sqlite3ErrorMsg(parse, "%s.%s exceeds indexes' names length "\
> +				"limit", table->def->name, name);

But sqlite3CheckIndentifier() also provides this check.

> +	uint32_t max_iid = 0;
> +	for (struct Index *index = table->pIndex; index != NULL;
> +	     index = index->pNext) {
> +		max_iid = max_iid > index->def->iid ?
> +			  max_iid : index->def->iid + 1;
> +	}

Look, you don’t need to find max_iid: if it is parsing stage, then you can
set it to any meaningful value (since in the end of function it is going to be destroyed);
if it is executing step, you can use db->init.newTnum.

> +
> +	bool is_unique = on_error != ON_CONFLICT_ACTION_NONE;

It seems to be so messy defining uniqueness by ON_CONFLICT_ACTION.
Lets refactor it somehow.

> @@ -460,17 +462,19 @@ fkLookupParent(Parse * pParse,	/* Parse context */
> 			 */
> 			if (pTab == pFKey->pFrom && nIncr == 1) {
> 				int iJump =
> -				    sqlite3VdbeCurrentAddr(v) + nCol + 1;
> -				for (i = 0; i < nCol; i++) {
> +					sqlite3VdbeCurrentAddr(v) + nCol + 1;
> +				struct key_part *part =
> +					pIdx->def->key_def->parts;
> +				for (i = 0; i < nCol; ++i, ++part) {
> 					int iChild = aiCol[i] + 1 + regData;
> -					int iParent =
> -					    pIdx->aiColumn[i] + 1 + regData;
> -					assert(pIdx->aiColumn[i] >= 0);
> +					int iParent = 1 + regData +
> +						      (int)part->fieldno;
> 					assert(aiCol[i] != pTab->iPKey);
> -					if (pIdx->aiColumn[i] == pTab->iPKey) {
> +					if ((int)part->fieldno == pTab->iPKey) {
> 						/* The parent key is a composite key that includes the IPK column */
> 						iParent = regData;
> 					}
> +

Extra empty line.

> @@ -622,7 +625,7 @@ fkScanChildren(Parse * pParse,	/* Parse context */
> 	Vdbe *v = sqlite3GetVdbe(pParse);
>  	assert(pIdx == 0 || pIdx->pTable == pTab);
> -	assert(pIdx == 0 || (int)index_column_count(pIdx) == pFKey->nCol);
> +	assert(pIdx == 0 || (int) pIdx->def->key_def->part_count == pFKey->nCol);

Lets use == NULL comparison on pointers.

> 
> @@ -1108,19 +1110,19 @@ sqlite3FkOldmask(Parse * pParse,	/* Parse context */
>  	if (user_session->sql_flags & SQLITE_ForeignKeys) {
> 		FKey *p;
> -		int i;
> 		for (p = pTab->pFKey; p; p = p->pNextFrom) {
> -			for (i = 0; i < p->nCol; i++)
> +			for (int i = 0; i < p->nCol; i++)

Is this change related to patch?

> 
> @@ -1390,24 +1389,22 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
> 		if (uniqueByteCodeNeeded) {
> 			sqlite3VdbeAddOp4Int(v, OP_NoConflict, iThisCur,
> 					     addrUniqueOk, regIdx,
> -					     index_column_count(pIdx));
> +					     pIdx->def->key_def->part_count);
> 		}
> 		VdbeCoverage(v);
> +		const uint32_t pk_part_count = pPk->def->key_def->part_count;

Why do you use here const? In other places AFAIK we/you don’t use
const modifier (when it comes to simple numeric variables).


> @@ -1621,15 +1621,12 @@ sql_stat4_column(struct sqlite3 *db, const char *record, uint32_t col_num,
> void
> sqlite3Stat4ProbeFree(UnpackedRecord * pRec)
> {
> -	if (pRec) {
> -		int i;
> -		int nCol = pRec->key_def->part_count;
> -		Mem *aMem = pRec->aMem;
> -		sqlite3 *db = aMem[0].db;
> -		for (i = 0; i < nCol; i++) {
> +	if (pRec != NULL) {
> +		int part_count = pRec->key_def->part_count;
> +		struct Mem *aMem = pRec->aMem;
> +		for (int i = 0; i < part_count; i++)
> 			sqlite3VdbeMemRelease(&aMem[i]);
> -		}
> -		sqlite3DbFree(db, pRec);
> +		sqlite3DbFree(aMem[0].db, pRec);
> 	}
> }

Is this refactoring related to patch? I mean, refactoring is always appreciated,
but don’t mess it with main goal of patch.

> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 85143ed20..7ca02095f 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -372,13 +372,19 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
> 	pScan->is_column_seen = false;
> 	if (pIdx) {
> 		int j = iColumn;
> -		iColumn = pIdx->aiColumn[j];
> +		iColumn = pIdx->def->key_def->parts[j].fieldno;
> +		/*
> +		 * pIdx->tnum == 0 means that pIdx is a fake
> +		 * integer primary key index.
> +		 */
> +		if (pIdx->tnum == 0)
> +			iColumn = -1;

We are going to remove tnum from struct Index and struct Table.
So, if it is possible, use index->def->iid instead (or smth else).

> 
> @@ -2882,7 +2868,6 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder,	/* WHERE clause information */
> 	Index *pProbe;		/* An index we are evaluating */
> 	Index sPk;		/* A fake index object for the primary key */
> 	LogEst aiRowEstPk[2];	/* The aiRowLogEst[] value for the sPk index */
> -	i16 aiColumnPk = -1;	/* The aColumn[] value for the sPk index */
> 	SrcList *pTabList;	/* The FROM clause */
> 	struct SrcList_item *pSrc;	/* The FROM clause btree term to add */
> 	WhereLoop *pNew;	/* Template WhereLoop object */
> @@ -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”,

Lets name if like ‘fake_autoindex’ to easily tell it from the rest.

> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation1.test.lua
> similarity index 100%
> rename from test/sql-tap/collation.test.lua
> rename to test/sql-tap/collation1.test.lua
> diff --git a/test/sql-tap/collation2.test.lua b/test/sql-tap/collation2.test.lua
> new file mode 100755
> index 000000000..64296b0be
> --- /dev/null
> +++ b/test/sql-tap/collation2.test.lua
> @@ -0,0 +1,20 @@
> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(3)
> +
> +test:do_catchsql_test(
> +        "collation-2.1",
> +        'CREATE TABLE test1 (a int, b int, c int, PRIMARY KEY (a, a, a, b, c))',
> +        nil)
> +
> +test:do_catchsql_test(
> +        "collation-2.2",
> +        'CREATE TABLE test2 (a int, b int, c int, PRIMARY KEY (a, a, a, b, b, a, c))',
> +        nil)
> +
> +test:do_catchsql_test(
> +        "collation-2.3",
> +        'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
> +        {1, "Collation 'FOO' does not exist"})
> +
> +test:finish_test()

I wouldn’t create separate test file for these simple tests.
Lets put them to existing one.