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

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

> On 9 Jul 2018, at 13:46, Ivan Koptelov <ivan.koptelov@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.