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 54EAE26DDA for ; Thu, 12 Jul 2018 07:18:16 -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 0IpF06gqUBOJ for ; Thu, 12 Jul 2018 07:18:16 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 DACE426DD9 for ; Thu, 12 Jul 2018 07:18:15 -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 v2] sql: add index_def to struct Index From: "n.pettik" In-Reply-To: <35c7947b-cb54-bac8-fed2-680c65f76cb6@tarantool.org> Date: Thu, 12 Jul 2018 14:18:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <35c7947b-cb54-bac8-fed2-680c65f76cb6@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 > On 9 Jul 2018, at 13:46, Ivan Koptelov = wrote: >=20 > Sorry, in both previous letters patch was not properly formatted. Now = I checked it > sending it to myself, it should be ok. If you want to comment smth, you should do it after commit message, near links to branch and issue - then, it would be omitted by git am. Also, again - you sent new version of patch, but didn=E2=80=99t attach = changelong. > =20 > -- > sql: add index_def to Index >=20 > 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, > aiRowLogEst and zName are removed from Index. All usages > of this fields changed to usage of corresponding index_def > or index_def->opts fields. > index_is_unique(), sql_index_collation() and > index_column_count() are removed with calls of > index_def corresponding fields. > Also there is small change in user-visible behavior: >=20 On the contrary, this behaviour is not visible for user, it is like things work under the hood. > before the patch a statement like > CREATE TABLE t1(a,b, PRIMARY KEY(a,b), UNIQUE(a,b)) > created only one constraint index (for primary key) > and no index for UNIQUE constraint (since it is upon the > same columns), neither it is named or non-named constraint. > After the patch index will be always created for named > constraints. It is a temporary solution. In future it's > preferable not to create an index, but to make some record > in _constraints space that this named unique constraint > implemented with the same index as primary key constraint. Not only PK constraints - any unique constraint. I have found that named PK over named UNIQUE is not created: create table t11(id int, constraint u1 unique(id), constraint pk1 = primary key (id)) tarantool> select * from =E2=80=9C_index" =E2=80=A6 - [350, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, = 'unsigned']]] - [512, 0, 'sql_autoindex_U1', 'tree', {'unique': true, 'sql': ''}, = [{'sort_order': 'asc', 'type': 'integer', 'is_nullable': false, 'nullable_action': = 'abort', 'field': 0}]] PK and UNIQUE - different constraints (I don=E2=80=99t even say that it = is named, so must be created without any doubts). > @@ -2511,44 +2431,11 @@ sqlite3RefillIndex(Parse * pParse, Index * = pIndex, int memRootPage) > sqlite3VdbeAddOp1(v, OP_Close, iSorter); > } > =20 > -/* > - * Allocate heap space to hold an Index object with nCol columns. > - * > - * Increase the allocation size to provide an extra nExtra bytes > - * of 8-byte aligned space after the Index object and return a > - * pointer to this extra space in *ppExtra. > - */ > -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 */ > - ) > +struct Index * > +sql_index_alloc(struct sqlite3 *db) > { > - 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; > - } > + int index_size =3D ROUND8(sizeof(struct Index)); > + struct Index *p =3D sqlite3DbMallocZero(db, index_size); >=20 I said you twice to remove this strange alignment: struct Index *p =3D sqlite3DbMallocZero(db, sizeof(*p)); > return p; > } > =20 > @@ -2635,48 +2522,132 @@ addIndexToTable(Index * pIndex, Table * pTab) >=20 > +static int > +index_fill_def(struct Parse *parse, struct Index *index, > + struct Table *table, uint32_t iid, const char *name, > + uint32_t name_len, struct ExprList *expr_list, > + enum sql_index_type idx_type, char *sql_stmt) > +{ > - return tnt_index->def->opts.is_unique; > + struct key_def *pk_key_def; > + if (idx_type =3D=3D SQL_INDEX_TYPE_UNIQUE || > + idx_type =3D=3D SQL_INDEX_TYPE_NON_UNIQUE) Why not !=3D SQL_INDEX_TYPE_CONSTRAINT_PK? > + pk_key_def =3D table->pIndex->def->key_def; > + else > + pk_key_def =3D NULL; > + > + index->def =3D index_def_new(space_def->id, iid, name, name_len, = TREE, > + &opts, key_def, pk_key_def); > + 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; > +tnt_error: > + parse->rc =3D SQL_TARANTOOL_ERROR; > + ++parse->nErr; > + goto cleanup; > } > =20 > 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, enum sql_index_type idx_type) { > + /* The index to be created. */ > + struct Index *index =3D NULL; > + /* Name of the index. */ > + char *name =3D NULL; > + struct sqlite3 *db =3D parse->db; > struct session *user_session =3D current_session(); > =20 > - 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) { > + if (!parse->nested && (idx_type =3D=3D SQL_INDEX_TYPE_UNIQUE || > + idx_type =3D=3D = SQL_INDEX_TYPE_NON_UNIQUE)) { > Vdbe *v =3D sqlite3GetVdbe(parse); > if (v =3D=3D NULL) > goto exit_create_index; > @@ -2685,39 +2656,30 @@ sql_create_index(struct Parse *parse, struct = Token *token, > assert(db->pSchema !=3D NULL); > =20 > /* > - * 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. > */ > + struct Table *table =3D NULL; > 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. > */ You have already deleted =E2=80=98fixing=E2=80=99 routine. Remove this = comment. > - assert(token && token->z); > - > - sqlite3FixInit(&sFix, parse, "index", token); > - if (sqlite3FixSrcList(&sFix, tbl_name)) { > - /* Because the parser constructs tbl_name from a = single identifier, > - * sqlite3FixSrcList can never fail. > - */ > - assert(0); > - } > - 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) > - goto exit_create_index; > - sqlite3PrimaryKeyIndex(pTab); > + assert(token !=3D NULL && token->z !=3D NULL); > + table =3D sqlite3LocateTable(parse, 0, = tbl_name->a[0].zName); > + assert(db->mallocFailed =3D=3D 0 || table =3D=3D NULL); > } else { > assert(token =3D=3D NULL); > assert(start =3D=3D NULL); > - pTab =3D parse->pNewTable; > - if (!pTab) > - goto exit_create_index; > + table =3D parse->pNewTable; > } > =20 > - assert(pTab !=3D 0); > - assert(parse->nErr =3D=3D 0); > - if (pTab->def->opts.is_view) { > + if (table =3D=3D NULL || parse->nErr > 0) > + goto exit_create_index; > + > + if (table->def->opts.is_view) { > sqlite3ErrorMsg(parse, "views may not be indexed"); > goto exit_create_index; > } > @@ -2734,45 +2696,68 @@ sql_create_index(struct Parse *parse, struct = Token *token, > * If token =3D=3D NULL it means that we are dealing with a > * primary key or UNIQUE constraint. We have to invent > * our own name. >=20 Fix also and this comment: it starts from: * Find the name of the index. Make sure there is not * already another index or table with the same name. * As we discussed, we allow index to be named as table. > + * > + * In case of UNIQUE constraint we have two options: > + * 1) UNIQUE constraint is named and this name will > + * be a part of index name. > + * 2) UNIQUE constraint is non-named and standard > + * auto-index name will be generated. > */ > - if (token) { > - zName =3D sqlite3NameFromToken(db, token); > - if (zName =3D=3D 0) > + bool is_constraint_named =3D false; > + if (token !=3D NULL) { > + name =3D sqlite3NameFromToken(db, token); > + if (name =3D=3D NULL) > goto exit_create_index; > - assert(token->z !=3D 0); > - if (!db->init.busy) { > - if (sqlite3HashFind(&db->pSchema->tblHash, = zName) !=3D > - NULL) { > - sqlite3ErrorMsg(parse, > - "there is already a = table named %s", > - zName); > - goto exit_create_index; > - } > - } > - if (sqlite3HashFind(&pTab->idxHash, zName) !=3D NULL) { > + assert(token->z !=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; > - pLoop =3D pLoop->pNext, n++) { > + char *constraint_name =3D NULL; > + if (parse->constraintName.z !=3D NULL) > + constraint_name =3D > + sqlite3NameFromToken(db, = &parse->constraintName); Out of 80. > + > + if (constraint_name =3D=3D NULL || > + strcmp(constraint_name, "") =3D=3D 0) { > + int n =3D 1; > + for (struct Index *idx =3D table->pIndex; > + idx !=3D NULL; > + idx =3D idx->pNext, n++) { > + } > + name =3D sqlite3MPrintf(db, = "sql_autoindex_%s_%d", > + table->def->name, n); > + } else { > + name =3D sqlite3MPrintf(db, "sql_autoindex_%s", > + constraint_name); > + is_constraint_named =3D true; Lets use another prefix than =E2=80=98sql_autoindex_=E2=80=99 for named = constraints, like =E2=80=98unique_constraint_=E2=80=99. And leave comment describing = this naming is temporary. > } > - zName =3D > - sqlite3MPrintf(db, "sqlite_autoindex_%s_%d", = pTab->def->name, > - n); > - if (zName =3D=3D 0) { > - goto exit_create_index; > + if (constraint_name !=3D NULL) { > + sqlite3DbFree(db, constraint_name); You don=E2=80=99t need this check: sqlite3DbFree tests on NULL ptr = itself. > + if (table =3D=3D parse->pNewTable) { > + for (struct Index *idx =3D table->pIndex; idx !=3D NULL; > + idx =3D idx->pNext) { > + uint32_t k; > + assert(IsUniqueIndex(idx)); > + assert(IsUniqueIndex(index)); > + > + if (idx->def->key_def->part_count !=3D > + index->def->key_def->part_count) > continue; > - for (k =3D 0; k < pIdx->nColumn; k++) { > - assert(pIdx->aiColumn[k] >=3D 0); > - if (pIdx->aiColumn[k] !=3D = pIndex->aiColumn[k]) > + for (k =3D 0; k < idx->def->key_def->part_count; = k++) { > + if (idx->def->key_def->parts[k].fieldno = !=3D > + = index->def->key_def->parts[k].fieldno) > break; > struct coll *coll1, *coll2; > - uint32_t id; > - coll1 =3D sql_index_collation(pIdx, k, = &id); > - coll2 =3D sql_index_collation(pIndex, k, = &id); > + coll1 =3D = idx->def->key_def->parts[k].coll; > + coll2 =3D = index->def->key_def->parts[k].coll; > if (coll1 !=3D coll2) > break; > } > - if (k =3D=3D pIdx->nColumn) { > - if (pIdx->onError !=3D pIndex->onError) = { > - /* This constraint creates the = same index as a previous > - * constraint specified = somewhere in the CREATE TABLE statement. > - * However the ON CONFLICT = clauses are different. If both this > - * constraint and the previous = equivalent constraint have explicit > - * ON CONFLICT clauses this is = an error. Otherwise, use the > - * explicitly specified behavior = for the index. > + if (k =3D=3D idx->def->key_def->part_count) { > + if (idx->onError !=3D index->onError) { > + /* > + * This constraint creates > + * the same index as a > + * previous > + * constraint specified > + * somewhere in the CREATE > + * TABLE statement. > + * However the ON CONFLICT > + * clauses are different. > + * If both this constraint > + * and the previous > + * equivalent constraint > + * have explicit > + * ON CONFLICT clauses > + * this is an error. > + * Otherwise, use the > + * explicitly specified > + * behavior for the index. > */ Cmon, it looks so ugly. Lets move this comment somewhere. Btw, AFAIU 66 is only recommendation, so in case of emergency you can = break this rule. > - if (! > - (pIdx->onError =3D=3D = ON_CONFLICT_ACTION_DEFAULT > - || pIndex->onError =3D=3D > - = ON_CONFLICT_ACTION_DEFAULT)) { > + if (idx->onError !=3D > + ON_CONFLICT_ACTION_DEFAULT = && > + index->onError !=3D > + ON_CONFLICT_ACTION_DEFAULT) = { > sqlite3ErrorMsg(parse, > - = "conflicting ON CONFLICT clauses specified", > - 0); > - } > - if (pIdx->onError =3D=3D = ON_CONFLICT_ACTION_DEFAULT) { > - pIdx->onError =3D = pIndex->onError; > + = "conflicting "\ > + "ON = CONFLICT "\ > + "clauses = "\ > + = "specified=E2=80=9D); Why do you continue processing here? > } > + if (idx->onError =3D=3D > + ON_CONFLICT_ACTION_DEFAULT) > + idx->onError =3D = index->onError; If you exit on error above, condition under this if statement will be always evaluated to true. > + } > + if (idx_type =3D=3D = SQL_INDEX_TYPE_CONSTRAINT_PK) > + idx->index_type =3D idx_type; Why do you need this if? You always assign idx->index_type =3D idx_type. > @@ -2880,9 +2865,7 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, = /* WHERE clause information */ > { > WhereInfo *pWInfo; /* WHERE analysis context */ > 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 = */ > + Index fake_index; /* A fake index object for the = primary key */ > SrcList *pTabList; /* The FROM clause */ > struct SrcList_item *pSrc; /* The FROM clause btree term to = add */ > WhereLoop *pNew; /* Template WhereLoop object */ > @@ -2903,31 +2886,62 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, = /* WHERE clause information */ > if (pSrc->pIBIndex) { > /* An INDEXED BY clause specifies a particular index to = use */ > pProbe =3D pSrc->pIBIndex; > + fake_index.def =3D NULL; > } else if (pTab->pIndex) { > pProbe =3D pTab->pIndex; > + fake_index.def =3D NULL; > } else { > /* There is no INDEXED BY clause. Create a fake Index = object in local > - * variable sPk to represent the primary key index. = Make this > + * variable fake_index to represent the primary key = index. Make this > * fake index the first in a chain of Index objects with = all of the real > * indices to follow > */ > 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; > - aiRowEstPk[0] =3D sql_space_tuple_log_count(pTab); > - aiRowEstPk[1] =3D 0; > + memset(&fake_index, 0, sizeof(Index)); > + fake_index.onError =3D ON_CONFLICT_ACTION_REPLACE; > + fake_index.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); > + > + struct index_opts opts; > + index_opts_create(&opts); > + opts.sql =3D "fake_autoindex"; > + fake_index.def =3D index_def_new(pTab->def->id, 0, = "fake_autoindex=E2=80=9D, Out of 80.