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

[n.pettik]

> +	/*
> +	 * Here we handle cases like
> +	 * CREATE TABLE t1(a UNIQUE, PRIMARY KEY(a))
> +	 * In these cases (where UNIQUE constraints precede
> +	 * PRIMARY constraint) appropriate Index->def->cmp_def
> +	 * can not be set 'on the fly' for indexes implementing
> +	 * UNIQUE constraints as PK key_def is missing and
> +	 * needed for it.
> +	 * At this point we can set appropriate cmp_def, since
> +	 * all indexes (from CREATE TABLE statement) is created.
> +	 */
> +	if (!p->def->opts.is_view) {
> +		assert(p->pIndex != NULL);
> +		struct Index *pk;
> +		for (struct Index *idx = p->pIndex; idx != NULL;
> +		     idx = idx->pNext) {
> +			if (idx->index_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
> +				pk = idx;
> +		}
> +		struct key_def *pk_def = pk->def->key_def;
> +
> +		for (struct Index *idx = p->pIndex; idx != NULL;
> +		     idx = idx->pNext) {
> +			if (idx->index_type != SQL_INDEX_TYPE_CONSTRAINT_PK) {
> +				struct index_def *def = idx->def;
> +				struct key_def *cmp_def =
> +					key_def_merge(def->key_def,
> +						      pk_def);
> +				if (cmp_def == NULL) {
> +					pParse->rc = SQL_TARANTOOL_ERROR;
> +					++pParse->nErr;
> +					return;
> +				}
> +				if (!def->opts.is_unique) {
> +					def->cmp_def->unique_part_count =
> +						def->cmp_def->part_count;
> +				} else {
> +					def->cmp_def->unique_part_count =
> +						def->key_def->part_count;
> +				}
> +			}
> +		}
> +	}
I don’t understand: why do you need at all this cmp_def?
In SQL it is extremely unlikely to be useful.
>  	if (db->init.busy) {
>  		/*
>  		 * As rebuild creates a new ExpList tree and
> @@ -2398,8 +2441,7 @@
>  struct Index *
>  sql_index_alloc(struct sqlite3 *db)
>  {
> -	int index_size = ROUND8(sizeof(struct Index));
> -	struct Index *p = sqlite3DbMallocZero(db, index_size);
> +	struct Index *p = sqlite3DbMallocZero(db, sizeof(struct Index));
>  	return p;
>  }
>  
> @@ -2566,12 +2608,25 @@
>  	if (parse->nErr > 0)
>  		goto cleanup;
>  
> -	struct key_def *pk_key_def;
> -	if (idx_type == SQL_INDEX_TYPE_UNIQUE ||
> -	    idx_type == SQL_INDEX_TYPE_NON_UNIQUE)
> +	struct key_def *pk_key_def = NULL;
> +	if (idx_type != SQL_INDEX_TYPE_CONSTRAINT_PK &&
> +	    parse->pNewTable == NULL)
>  		pk_key_def = table->pIndex->def->key_def;
> -	else
> -		pk_key_def = NULL;
> +	/*
> +	 * In cases like CREATE TABLE t1(a UNIQUE, PRIMARY KEY(a))
> +	 * fill_index_def() will be firstly executed for creating
> +	 * index_def for UNIQUE constraint and then for creating
> +	 * index_def for PRIMARY KEY constraint. So when
> +	 * fill_index_def will be called for UNIQUE constraint
> +	 * there will be no PK, while PK->def->key_def is needed
> +	 * to create cmp_def (inside index_def_new()). 
Again: explain pls why do you need cmp_def on client-side (i.e. parser)?
> To handle
> +	 * this case we set here pk_key_def to key_def and later
> +	 * in sqlite3EndTable (when PK index is already created)
> +	 * we go over all indexes and set appropriate cmp_def in
> +	 * them.
> +	 */
> +	if (parse->pNewTable != NULL)
> +		pk_key_def = key_def;
>  
>  	index->def = index_def_new(space_def->id, iid, name, name_len, TREE,
>  				   &opts, key_def, pk_key_def);
> @@ -2618,12 +2673,6 @@
>  	 */
>  	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.
> -		 */
>  		assert(token != NULL && token->z != NULL);
>  		table = sqlite3LocateTable(parse, 0, tbl_name->a[0].zName);
>  		assert(db->mallocFailed == 0 || table == NULL);
> @@ -2642,7 +2691,7 @@
>  	}
>  	/*
>  	 * Find the name of the index.  Make sure there is not
> -	 * already another index or table with the same name.
> +	 * already another index with the same name.
>  	 *
>  	 * Exception:  If we are reading the names of permanent
>  	 * indices from the Tarantool schema (because some other
> @@ -2660,7 +2709,7 @@
>  	 * 2) UNIQUE constraint is non-named and standard
>  	 *    auto-index name will be generated.
>  	 */
> -	bool is_constraint_named = false;
> +	bool is_constraint_named;
>  	if (token != NULL) {
>  		name = sqlite3NameFromToken(db, token);
>  		if (name == NULL)
> @@ -2691,14 +2740,26 @@
>  			}
>  			name = sqlite3MPrintf(db, "sql_autoindex_%s_%d",
>  					      table->def->name, n);
> +			is_constraint_named = false;
>  		} else {
> -			name = sqlite3MPrintf(db, "sql_autoindex_%s",
> -					      constraint_name);
> +			/*
> +			 * This naming is temporary. Now it's not
> +			 * possible (since we implement UNIQUE
> +			 * and PK constraints with indexes and
> +			 * indexes can not have same names), but
> +			 * in future we would use names exactly
> +			 * as they are set by user.
> +			 */
> +			if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)
> +				name = sqlite3MPrintf(db,
> +						      "unique_constraint_%s",
> +						      constraint_name);
> +			if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK)
> +				name = sqlite3MPrintf(db, "pk_constraint_%s",
> +						      constraint_name);
>  			is_constraint_named = true;
>  		}
> -		if (constraint_name != NULL) {
> -			sqlite3DbFree(db, constraint_name);
> -		}
> +		sqlite3DbFree(db, constraint_name);
>  	}
>  
>  	if (name == NULL)
> @@ -2708,7 +2769,8 @@
>  			       table->def->id < BOX_SYSTEM_ID_MAX;
>  	if (is_system_space && (idx_type == SQL_INDEX_TYPE_NON_UNIQUE ||
>  				idx_type == SQL_INDEX_TYPE_UNIQUE)) {
> -		diag_set(ClientError, ER_MODIFY_INDEX, name, table->def->name,
> +		diag_set(ClientError, ER_MODIFY_INDEX, name,
> +			 table->def->name,
>  			 "can't create index on system space");
>  		parse->nErr++;
>  		parse->rc = SQL_TARANTOOL_ERROR;
> @@ -2745,7 +2807,7 @@
>  
>  	index->pTable = table;
>  	index->onError = (u8) on_error;
> - 	index->index_type = idx_type;
> +	index->index_type = idx_type;
>  	index->pSchema = db->pSchema;
>  	/* Tarantool have access to each column by any index. */
>  	if (where != NULL) {
> @@ -2775,8 +2837,22 @@
>  			goto exit_create_index;
>  	}
>  
> -	/* If it is parsing stage, then iid may have any value. */
> -	uint32_t iid = 1;
> +	/*
> +	 * We set no SQL statement for indexes created during
> +	 * CREATE TABLE statement. Instead we use
> +	 * Index->index_def->opts.sql to store information if
> +	 * this index implements named or non-named constraint.
> +	 */
> +	if (tbl_name == NULL &&
> +	    idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE)
If idx_type == SQL_INDEX_TYPE_CONSTRAINT_UNIQUE than tbl_name must be NULL:
you can’t declare unique constraint outside CREATE TABLE statement.

> +		sql_stmt = is_constraint_named ?
> +			   "named constraint" : "non-named constraint”;
You can set ‘sql_stmt’ only for name constraints:

sql_stmt = is_constraint_name ? “_named_constr” :  “”;

And use strncmp when comparing with it. 

Also, if <if-clause> contains mode than one row,
we wrap it in brackets.
> +
> +	/*
> +	 * If it is parsing stage, then iid may have any value,
> +	 * but PK still must have iid == 0
> +	 */
> +	uint32_t iid = idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK ? 0 : 1;
>  	if (db->init.busy)
>  		iid = SQLITE_PAGENO_TO_INDEXID(db->init.newTnum);
You can just do this:

uint32_t iid = idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK;
> +
> +			if (k != key_def->part_count)
> +				continue;
> +
> +			if (index->onError != existing_idx->onError) {
> +				if (index->onError !=
> +				    ON_CONFLICT_ACTION_DEFAULT &&
> +				    existing_idx->onError !=
> +				    ON_CONFLICT_ACTION_DEFAULT)
> +					sqlite3ErrorMsg(parse,
> +							"conflicting "\
> +							"ON CONFLICT "\
> +							"clauses specified”);
Again: here you have already set error. What is the reason to
continue process smth? You are able to go to exit_create_index.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180713/e2c8b138/attachment.html>