[tarantool-patches] Re: [PATCH v2] sql: add index_def to struct Index
n.pettik
korablev at tarantool.org
Thu Jul 12 14:18:09 MSK 2018
> On 9 Jul 2018, at 13:46, Ivan Koptelov <ivan.koptelov at tarantool.org> wrote:
>
> 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’t attach changelong.
>
> --
> 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,
> 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:
>
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 “_index"
…
- [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’t 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);
> }
>
> -/*
> - * 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 = 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;
> - }
> + int index_size = ROUND8(sizeof(struct Index));
> + struct Index *p = sqlite3DbMallocZero(db, index_size);
>
I said you twice to remove this strange alignment:
struct Index *p = sqlite3DbMallocZero(db, sizeof(*p));
> return p;
> }
>
> @@ -2635,48 +2522,132 @@ addIndexToTable(Index * pIndex, Table * pTab)
>
> +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 == SQL_INDEX_TYPE_UNIQUE ||
> + idx_type == SQL_INDEX_TYPE_NON_UNIQUE)
Why not != SQL_INDEX_TYPE_CONSTRAINT_PK?
> + pk_key_def = table->pIndex->def->key_def;
> + else
> + pk_key_def = NULL;
> +
> + index->def = index_def_new(space_def->id, iid, name, name_len, TREE,
> + &opts, key_def, pk_key_def);
> + if (index->def == NULL)
> + goto tnt_error;
> + rc = 0;
> +cleanup:
> + if (key_def != NULL)
> + key_def_delete(key_def);
> + return rc;
> +tnt_error:
> + parse->rc = SQL_TARANTOOL_ERROR;
> + ++parse->nErr;
> + goto cleanup;
> }
>
> 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, enum sql_index_type idx_type) {
> + /* The index to be created. */
> + struct Index *index = NULL;
> + /* Name of the index. */
> + char *name = NULL;
> + 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) {
> + if (!parse->nested && (idx_type == SQL_INDEX_TYPE_UNIQUE ||
> + idx_type == SQL_INDEX_TYPE_NON_UNIQUE)) {
> Vdbe *v = sqlite3GetVdbe(parse);
> if (v == NULL)
> goto exit_create_index;
> @@ -2685,39 +2656,30 @@ 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.
> */
> + struct Table *table = NULL;
> 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.
> */
You have already deleted ‘fixing’ 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 = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
> - assert(db->mallocFailed == 0 || pTab == 0);
> - if (pTab == 0)
> - goto exit_create_index;
> - sqlite3PrimaryKeyIndex(pTab);
> + assert(token != NULL && token->z != NULL);
> + table = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
> + assert(db->mallocFailed == 0 || table == NULL);
> } else {
> assert(token == NULL);
> assert(start == NULL);
> - pTab = parse->pNewTable;
> - if (!pTab)
> - goto exit_create_index;
> + table = parse->pNewTable;
> }
>
> - assert(pTab != 0);
> - assert(parse->nErr == 0);
> - if (pTab->def->opts.is_view) {
> + if (table == 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 == NULL it means that we are dealing with a
> * primary key or UNIQUE constraint. We have to invent
> * our own name.
>
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 = sqlite3NameFromToken(db, token);
> - if (zName == 0)
> + bool is_constraint_named = false;
> + if (token != NULL) {
> + name = sqlite3NameFromToken(db, token);
> + if (name == NULL)
> goto exit_create_index;
> - assert(token->z != 0);
> - if (!db->init.busy) {
> - if (sqlite3HashFind(&db->pSchema->tblHash, zName) !=
> - NULL) {
> - sqlite3ErrorMsg(parse,
> - "there is already a table named %s",
> - zName);
> - goto exit_create_index;
> - }
> - }
> - if (sqlite3HashFind(&pTab->idxHash, zName) != NULL) {
> + assert(token->z != 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;
> - pLoop = pLoop->pNext, n++) {
> + char *constraint_name = NULL;
> + if (parse->constraintName.z != NULL)
> + constraint_name =
> + sqlite3NameFromToken(db, &parse->constraintName);
Out of 80.
> +
> + if (constraint_name == NULL ||
> + strcmp(constraint_name, "") == 0) {
> + int n = 1;
> + for (struct Index *idx = table->pIndex;
> + idx != NULL;
> + idx = idx->pNext, n++) {
> + }
> + name = sqlite3MPrintf(db, "sql_autoindex_%s_%d",
> + table->def->name, n);
> + } else {
> + name = sqlite3MPrintf(db, "sql_autoindex_%s",
> + constraint_name);
> + is_constraint_named = true;
Lets use another prefix than ‘sql_autoindex_’ for named constraints,
like ‘unique_constraint_’. And leave comment describing this naming is temporary.
> }
> - zName =
> - sqlite3MPrintf(db, "sqlite_autoindex_%s_%d", pTab->def->name,
> - n);
> - if (zName == 0) {
> - goto exit_create_index;
> + if (constraint_name != NULL) {
> + sqlite3DbFree(db, constraint_name);
You don’t need this check: sqlite3DbFree tests on NULL ptr itself.
> + if (table == parse->pNewTable) {
> + for (struct Index *idx = table->pIndex; idx != NULL;
> + idx = idx->pNext) {
> + uint32_t k;
> + assert(IsUniqueIndex(idx));
> + assert(IsUniqueIndex(index));
> +
> + if (idx->def->key_def->part_count !=
> + index->def->key_def->part_count)
> continue;
> - for (k = 0; k < pIdx->nColumn; k++) {
> - assert(pIdx->aiColumn[k] >= 0);
> - if (pIdx->aiColumn[k] != pIndex->aiColumn[k])
> + for (k = 0; k < idx->def->key_def->part_count; k++) {
> + if (idx->def->key_def->parts[k].fieldno !=
> + index->def->key_def->parts[k].fieldno)
> break;
> struct coll *coll1, *coll2;
> - uint32_t id;
> - coll1 = sql_index_collation(pIdx, k, &id);
> - coll2 = sql_index_collation(pIndex, k, &id);
> + coll1 = idx->def->key_def->parts[k].coll;
> + coll2 = index->def->key_def->parts[k].coll;
> if (coll1 != coll2)
> break;
> }
> - if (k == pIdx->nColumn) {
> - if (pIdx->onError != 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 == idx->def->key_def->part_count) {
> + if (idx->onError != 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 == ON_CONFLICT_ACTION_DEFAULT
> - || pIndex->onError ==
> - ON_CONFLICT_ACTION_DEFAULT)) {
> + if (idx->onError !=
> + ON_CONFLICT_ACTION_DEFAULT &&
> + index->onError !=
> + ON_CONFLICT_ACTION_DEFAULT) {
> sqlite3ErrorMsg(parse,
> - "conflicting ON CONFLICT clauses specified",
> - 0);
> - }
> - if (pIdx->onError == ON_CONFLICT_ACTION_DEFAULT) {
> - pIdx->onError = pIndex->onError;
> + "conflicting "\
> + "ON CONFLICT "\
> + "clauses "\
> + "specified”);
Why do you continue processing here?
> }
> + if (idx->onError ==
> + ON_CONFLICT_ACTION_DEFAULT)
> + idx->onError = index->onError;
If you exit on error above, condition under this if statement
will be always evaluated to true.
> + }
> + if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
> + idx->index_type = idx_type;
Why do you need this if? You always assign idx->index_type = 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 = -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 = pSrc->pIBIndex;
> + fake_index.def = NULL;
> } else if (pTab->pIndex) {
> pProbe = pTab->pIndex;
> + fake_index.def = 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 = 1;
> - sPk.aiColumn = &aiColumnPk;
> - sPk.aiRowLogEst = aiRowEstPk;
> - sPk.onError = ON_CONFLICT_ACTION_REPLACE;
> - sPk.pTable = pTab;
> - aiRowEstPk[0] = sql_space_tuple_log_count(pTab);
> - aiRowEstPk[1] = 0;
> + memset(&fake_index, 0, sizeof(Index));
> + fake_index.onError = ON_CONFLICT_ACTION_REPLACE;
> + fake_index.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);
> +
> + struct index_opts opts;
> + index_opts_create(&opts);
> + opts.sql = "fake_autoindex";
> + fake_index.def = index_def_new(pTab->def->id, 0, "fake_autoindex”,
Out of 80.
More information about the Tarantool-patches
mailing list