From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D7CBA26F9C for ; Tue, 3 Jul 2018 19:54:48 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5OsNVbntyODI for ; Tue, 3 Jul 2018 19:54:48 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E24CF267CC for ; Tue, 3 Jul 2018 19:54:47 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v9] sql: add index_def to struct Index From: "n.pettik" In-Reply-To: <2d4908aa-0243-8dc3-e109-707cb482b7f6@tarantool.org> Date: Wed, 4 Jul 2018 02:54:39 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <9A0A687D-2E0B-4AB9-B223-1A1287817824@tarantool.org> <8900ae2a-59e8-d69a-7f5d-95436b23214a@tarantool.org> <0e795fb5-1ee8-e7e6-65e4-7347787f3248@tarantool.org> <898ec1ac-51b9-13a6-6298-3a20eeac5b2b@tarantool.org> <76c1ec30-4f33-bc91-8845-72a99fc4ef0f@tarantool.org> <6fbc3849-a204-6cd9-82cd-2fb22769ccf0@tarantool.org> <2d4908aa-0243-8dc3-e109-707cb482b7f6@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Ivan Koptelov 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) --- =E2=80=A6 No error is raised and index isn=E2=80=99t 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 =3D=3D (regStat4 + 2)); > Index *pPk =3D sqlite3PrimaryKeyIndex(pIdx->pTable); > - int j, k, regKeyStat; > - int nPkColumn =3D (int)index_column_count(pPk); > - regKeyStat =3D sqlite3GetTempRange(pParse, nPkColumn); > - for (j =3D 0; j < nPkColumn; j++) { > - k =3D pPk->aiColumn[j]; > - assert(k >=3D 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 =3D (int) = pPk->def->key_def->part_count; > + int regKeyStat =3D sqlite3GetTempRange(pParse, = pk_part_count); > + for (int j =3D 0; j < pk_part_count; ++j) { Use uint32_t instead of int: - int pk_part_count =3D (int) = pPk->def->key_def->part_count; + uint32_t pk_part_count =3D = pPk->def->key_def->part_count; int regKeyStat =3D sqlite3GetTempRange(pParse, = pk_part_count); - for (int j =3D 0; j < pk_part_count; ++j) { - int k =3D pPk->def->key_def->parts[j].fieldno; - assert(k >=3D 0 && k < (int) = pTab->def->field_count); + for (uint32_t j =3D 0; j < pk_part_count; ++j) { + uint32_t k =3D = pPk->def->key_def->parts[j].fieldno; + assert(k >=3D 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 =3D 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 =3D sqlite3DbMallocZero(db, nByte + nExtra); > - if (p) { > - char *pExtra =3D ((char *)p) + ROUND8(sizeof(Index)); > - p->coll_array =3D (struct coll **)pExtra; > - pExtra +=3D ROUND8(sizeof(struct coll **) * nCol); > - p->coll_id_array =3D (uint32_t *) pExtra; > - pExtra +=3D ROUND8(sizeof(uint32_t) * nCol); > - p->aiRowLogEst =3D (LogEst *) pExtra; > - pExtra +=3D sizeof(LogEst) * (nCol + 1); > - p->aiColumn =3D (i16 *) pExtra; > - pExtra +=3D sizeof(i16) * nCol; > - p->sort_order =3D (enum sort_order *) pExtra; > - p->nColumn =3D nCol; > - *ppExtra =3D ((char *)p) + nByte; > - } > + /* Size of struct Index and aiRowLogEst. */ > + int nByte =3D ROUND8(sizeof(struct Index)) + > + ROUND8(sizeof(LogEst) * (part_count + 1)); Do we really need this alignment? > + struct Index *p =3D sqlite3DbMallocZero(db, nByte); > + if (p !=3D NULL) > + p->aiRowLogEst =3D (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 =3D strlen(str); > + char *str_part =3D region_alloc(&parse->region, str_len); > + if (str_part =3D=3D NULL) { > + diag_set(OutOfMemory, str_len, "region_alloc", = "str_part"); > + parse->rc =3D 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 =3D table->def; > + size_t sql_size =3D 0; > + struct index_opts opts; > + index_opts_create(&opts); > + index->def =3D NULL; > + opts.is_unique =3D is_unique; > + int rc =3D -1; You don=E2=80=99t use this variable and in the end just reassign it: + if (index->def =3D=3D NULL) + goto tnt_error; + rc =3D 0; +cleanup: + if (key_def !=3D NULL) + key_def_delete(key_def); + return rc; > + > + struct key_def *key_def =3D key_def_new(expr_list->nExpr); > + if (key_def =3D=3D NULL) > + goto tnt_error; > + > + /* Build initial parts of SQL statement. */ > + if (idx_type =3D=3D SQLITE_IDXTYPE_APPDEF) { > + sql_size +=3D sql_append(parse, "CREATE INDEX "); > + sql_size +=3D sql_append(parse, name); > + sql_size +=3D sql_append(parse, " ON "); > + sql_size +=3D sql_append(parse, space_def->name); > + sql_size +=3D sql_append(parse, " (=E2=80=9C); Why do you need to construct =E2=80=9CCREATE INDEX=E2=80=9D 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 !=3D NULL); int n =3D (int)(parse->sLastToken.z - token->z) + parse->sLastToken.n; if (token->z[n - 1] =3D=3D ';') 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 =3D 0; /* Table to be indexed */ > - Index *pIndex =3D 0; /* The index to be created */ > - char *zName =3D 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 =3D parse->db; > - struct ExprList_item *col_listItem; /* For looping over = col_list */ > - int nExtra =3D 0; /* Space allocated for zExtra[] = */ > - char *zExtra =3D 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 =3D NULL; > + /* The index to be created. */ > + struct Index *index =3D NULL; > + /* Name of the index. */ > + char *name =3D NULL; > + int name_len; You don=E2=80=99t need to declare variables so beforehand. Its first usage occurs at 130+ lines below. > + struct sqlite3 *db =3D parse->db; > struct session *user_session =3D 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 =3D=3D SQLITE_IDXTYPE_APPDEF) { > Vdbe *v =3D sqlite3GetVdbe(parse); > @@ -2681,39 +2711,43 @@ sql_create_index(struct Parse *parse, struct = Token *token, > assert(db->pSchema !=3D 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 !=3D 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 =E2=80=98Fix=E2=80=99 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 =3D sqlite3LocateTable(parse, 0, = tbl_name->a[0].zName); > - assert(db->mallocFailed =3D=3D 0 || pTab =3D=3D 0); > - if (pTab =3D=3D 0) > + table =3D sqlite3LocateTable(parse, 0, = tbl_name->a[0].zName); > + assert(db->mallocFailed =3D=3D 0 || table =3D=3D NULL); > + if (table =3D=3D NULL) > goto exit_create_index; > - sqlite3PrimaryKeyIndex(pTab); > + sqlite3PrimaryKeyIndex(table); Why do you call this function? It returns PK, but you don=E2=80=99t use = it. > } else { > assert(token =3D=3D NULL); > assert(start =3D=3D NULL); > - pTab =3D parse->pNewTable; > - if (!pTab) > + table =3D parse->pNewTable; > + if (table =3D=3D NULL) > goto exit_create_index; > } Instead of checking table on NULL in each branch and after that using = assert(table !=3D NULL), it is better to replace that assert() with check: - if (table =3D=3D NULL) - goto exit_create_index; - sqlite3PrimaryKeyIndex(table); } else { assert(token =3D=3D NULL); assert(start =3D=3D NULL); table =3D parse->pNewTable; - if (table =3D=3D NULL) - goto exit_create_index; } =20 - assert(table !=3D NULL); + if (table =3D=3D NULL) + goto exit_create_index; > - assert(pTab !=3D 0); > + assert(table !=3D NULL); > assert(parse->nErr =3D=3D 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 =3D sqlite3NameFromToken(db, token); > - if (zName =3D=3D 0) > + if (token !=3D NULL) { > + name =3D sqlite3NameFromToken(db, token); > + if (name =3D=3D NULL) > goto exit_create_index; > - assert(token->z !=3D 0); > + assert(token->z !=3D NULL); > if (!db->init.busy) { > - if (sqlite3HashFind(&db->pSchema->tblHash, = zName) !=3D > + if (sqlite3HashFind(&db->pSchema->tblHash, name) = !=3D > 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) !=3D NULL) { > + if (sqlite3HashFind(&table->idxHash, name) !=3D 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 =3D pTab->pIndex, n =3D 1; pLoop; > + int n =3D 1; > + for (struct Index *pLoop =3D table->pIndex; pLoop !=3D = NULL; Lets use Tarantool naming convention. > pLoop =3D pLoop->pNext, n++) { > } > - zName =3D > - sqlite3MPrintf(db, "sqlite_autoindex_%s_%d", = pTab->def->name, > - n); > - if (zName =3D=3D 0) { > + name =3D sqlite3MPrintf(db, "sqlite_autoindex_%s_%d=E2=80=9D= , Lets remove =E2=80=99sqlite_=E2=80=99 prefix and use just =E2=80=99sql_=E2= =80=99. > + table->def->name, n); > + if (name =3D=3D NULL) > goto exit_create_index; > - } > } > /* > @@ -2776,9 +2806,9 @@ sql_create_index(struct Parse *parse, struct = Token *token, > * simulate this. > */ > if (col_list =3D=3D NULL) { > - Token prevCol; > - uint32_t last_field =3D 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 =3D table->def->field_count - 1; > + sqlite3TokenInit(&prevCol, = table->def->fields[last_field].name); > col_list =3D 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 =3D 0; i < col_list->nExpr; i++) { > - Expr *pExpr =3D col_list->a[i].pExpr; > - assert(pExpr !=3D 0); > - if (pExpr->op =3D=3D TK_COLLATE) { > - nExtra +=3D (1 + = sqlite3Strlen30(pExpr->u.zToken)); > - } > - } > + /* Allocate the index structure. */ Extra space after dot. > + name_len =3D sqlite3Strlen30(name); Lets use traditional strlen() instead of SQLite analogs. > - /* > - * Allocate the index structure. > - */ > - nName =3D sqlite3Strlen30(zName); > - pIndex =3D 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 =3D 0; > + for (struct Index *index =3D table->pIndex; index !=3D NULL; > + index =3D index->pNext) { > + max_iid =3D max_iid > index->def->iid ? > + max_iid : index->def->iid + 1; > + } Look, you don=E2=80=99t 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 =3D on_error !=3D 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 =3D=3D pFKey->p=46rom && nIncr =3D=3D = 1) { > int iJump =3D > - sqlite3VdbeCurrentAddr(v) + nCol + = 1; > - for (i =3D 0; i < nCol; i++) { > + sqlite3VdbeCurrentAddr(v) + nCol = + 1; > + struct key_part *part =3D > + pIdx->def->key_def->parts; > + for (i =3D 0; i < nCol; ++i, ++part) { > int iChild =3D aiCol[i] + 1 + = regData; > - int iParent =3D > - pIdx->aiColumn[i] + 1 + = regData; > - assert(pIdx->aiColumn[i] >=3D = 0); > + int iParent =3D 1 + regData + > + = (int)part->fieldno; > assert(aiCol[i] !=3D = pTab->iPKey); > - if (pIdx->aiColumn[i] =3D=3D = pTab->iPKey) { > + if ((int)part->fieldno =3D=3D = pTab->iPKey) { > /* The parent key is a = composite key that includes the IPK column */ > iParent =3D regData; > } > + Extra empty line. > @@ -622,7 +625,7 @@ fkScanChildren(Parse * pParse, /* Parse context = */ > Vdbe *v =3D sqlite3GetVdbe(pParse); > assert(pIdx =3D=3D 0 || pIdx->pTable =3D=3D pTab); > - assert(pIdx =3D=3D 0 || (int)index_column_count(pIdx) =3D=3D = pFKey->nCol); > + assert(pIdx =3D=3D 0 || (int) pIdx->def->key_def->part_count =3D=3D= pFKey->nCol); Lets use =3D=3D NULL comparison on pointers. >=20 > @@ -1108,19 +1110,19 @@ sqlite3FkOldmask(Parse * pParse, /* Parse = context */ > if (user_session->sql_flags & SQLITE_ForeignKeys) { > FKey *p; > - int i; > for (p =3D pTab->pFKey; p; p =3D p->pNextFrom) { > - for (i =3D 0; i < p->nCol; i++) > + for (int i =3D 0; i < p->nCol; i++) Is this change related to patch? >=20 > @@ -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 =3D = pPk->def->key_def->part_count; Why do you use here const? In other places AFAIK we/you don=E2=80=99t = 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 =3D pRec->key_def->part_count; > - Mem *aMem =3D pRec->aMem; > - sqlite3 *db =3D aMem[0].db; > - for (i =3D 0; i < nCol; i++) { > + if (pRec !=3D NULL) { > + int part_count =3D pRec->key_def->part_count; > + struct Mem *aMem =3D pRec->aMem; > + for (int i =3D 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=E2=80=99t 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 =3D false; > if (pIdx) { > int j =3D iColumn; > - iColumn =3D pIdx->aiColumn[j]; > + iColumn =3D pIdx->def->key_def->parts[j].fieldno; > + /* > + * pIdx->tnum =3D=3D 0 means that pIdx is a fake > + * integer primary key index. > + */ > + if (pIdx->tnum =3D=3D 0) > + iColumn =3D -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). >=20 > @@ -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 =3D -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 =3D 1; > - sPk.aiColumn =3D &aiColumnPk; > sPk.aiRowLogEst =3D aiRowEstPk; > sPk.onError =3D ON_CONFLICT_ACTION_REPLACE; > sPk.pTable =3D pTab; > + > + struct key_def *key_def =3D key_def_new(1); > + if (key_def =3D=3D NULL) { > + pWInfo->pParse->nErr++; > + pWInfo->pParse->rc =3D 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 =3D index_def_new(pTab->def->id, 0, "primary=E2=80= =9D, Lets name if like =E2=80=98fake_autoindex=E2=80=99 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 =3D 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=E2=80=99t create separate test file for these simple tests. Lets put them to existing one.