[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index (fixed version)

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the patch! See my 35 comments below.

1. The patch is not so big to do not fit in a letter. Please, send a patch as a file only when
it is really huge.

2. There is a problem with your GibHub account. Your profile name in commits on github must
be a link to the profile. But your name is just a text. Please, cope with the problem.

3. Now you can remove sql_index_collation() function. It is useless two line wrapper. And
its second out parameter is not needed in some places.

4. Same about index_is_unique().

5. get_new_iid() is unused.

> +void
> +append(struct region *r, const char *str, char **sql_arr, int idx, int total_sql_str_size)
> +{
> +	sql_arr[idx] = region_alloc(r, strlen(str));
> +	strcpy(sql_arr[idx], str);
> +	total_sql_str_size += strlen(str);
> +}

6. How does it work? total_sql_str_size is not out parameter here. In the caller
context it is still unchanged.

> +
> +char *
> +create_sql(const char *idx_name, struct space_def *space_def, ExprList *expr_list)
> +{
> +	struct region *r = &fiber()->gc;
> +	/*
> +	 * CREATE_INDEX_ + INDEX_NAME + _ON_ + TABLE_NAME + _(
> +	 */
> +	uint32_t sql_parts_count = 5;

7. You do not need sql parts array, part count. You just do region_alloc + memcpy and
region_join at the end. It is not required to store each part anywhere.

> +	Expr *column_expr;
> +	for (uint32_t i = 0; i < (uint32_t) expr_list->nExpr; i++){
> +		column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> +		sql_parts_count += expr_list->a[i].pExpr == column_expr ? 2 : 4;
> +	}
> +
> +	size_t total_sql_str_size = 0;
> +
> +	char **sql = region_alloc(r, sizeof(char*) * sql_parts_count);
> +
> +	char *CREATE_INDEX = "CREATE INDEX ";

8. Do not store const string as non-const.

9. Why do you need this variable? Why can not you just
inline it one line below?.

> +	append(r, CREATE_INDEX, sql, 0, total_sql_str_size);
> +	append(r, idx_name, sql, 1, total_sql_str_size);
> +
> +	char *ON = " ON ";

10. Same.

> +	append(r, ON, sql, 2, total_sql_str_size);
> +
> +	char *name = space_def->name;
> +	append(r, name, sql, 3, total_sql_str_size);
> +	append(r, " (", sql, 4, total_sql_str_size);
> +
> +	Expr *collation_expr;
> +	uint32_t sql_idx = 5;
> +	for (int i = 0; i < expr_list->nExpr; i++){
> +		column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> +		collation_expr = expr_list->a[i].pExpr == column_expr ?
> +				 NULL : expr_list->a[i].pExpr;

11. Please, do like sql_create_index(): use pExpr->op == TK_COLLATE to detect that
it is a collated column.

12. Why did not you call sqlite3ResolveSelfReference() ?

13. Why your patch did not delete Index.coll_array, Index.coll_id_array, Index.aiColumn?
Your patch must remove them. Index_def initialization must replace code in build.c
on lines 3158:3202.

> +	// fix last ", " with ")\0"
> +	strcpy(sql[sql_idx - 1], ")\0");
> +	char *res = region_join(r, total_sql_str_size);
> +	return res;

14. Do not use // comments.

15. Why do you need separate create_sql() function? Can you generate an expression
during parts initialization?

> +void
> +set_index_def(Parse *parse, Index *index, Table *table,
> +				   const char *name,
> +				   uint32_t name_len, int on_error,
> +				   ExprList *expr_list, u8 idx_type)

16. Big problems with indentation.

> +{
> +	struct space_def *space_def = table->def;
> +	uint32_t iid = SQLITE_PAGENO_TO_INDEXID(index->tnum);

17. You do not know iid here. The parser just parses and does not
generate identifiers. I checked this code in a debugger, and in this example:

box.sql.execute("CREATE TABLE t1(x integer primary key, a integer unique);")

It creates two struct Index with the same def->iid == 0. It works only thanks
to that Index.def->iid is not used anywhere now.

> +	struct index_opts opts;
> +	index_opts_create(&opts);
> +	opts.stat = NULL;

18. Already done by index_opts_create.

> +	opts.is_unique = on_error != ON_CONFLICT_ACTION_NONE;
> +
> +	struct key_def *key_def = key_def_new(expr_list->nExpr);

19. Please, check each function result on error. Key_def_new can
fail.

> +
> +	if (idx_type == SQLITE_IDXTYPE_APPDEF) {
> +		opts.sql = create_sql(name, table->def, expr_list);
> +	}

20. Please, do not put 'if' body in {} when it consists of a single line.

> +
> +	int i = 0;

21. Why can not you put 'int i = 0' into 'for (...'?

> +	Expr *column_expr;
> +	Expr *collation_expr;

22. Same. These variables are unused out of 'for'. And can be declared +
initialized inside.

> +	for (i = 0; i < expr_list->nExpr; i++) {
> +		column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> +		collation_expr = expr_list->a[i].pExpr == column_expr ?
> +				 NULL : expr_list->a[i].pExpr;

23. Same as 11.

> +
> +		uint32_t fieldno = column_expr->iColumn;
> +
> +		uint32_t coll_id;
> +		struct coll *coll;
> +		if (collation_expr != NULL)
> +			coll = sql_get_coll_seq(parse, collation_expr->u.zToken,&coll_id);
> +		else
> +			coll = sql_column_collation(space_def, fieldno, &coll_id);
> +
> +		key_def_set_part(key_def, i, fieldno,
> +				space_def->fields[fieldno].type,
> +				space_def->fields[fieldno].nullable_action,
> +				coll, coll_id, SORT_ORDER_ASC);
> +	}
> +
> +	struct key_def *pk_key_def;
> +	if (idx_type == SQLITE_IDXTYPE_APPDEF) {
> +		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);

24. Same as 16.

> @@ -3068,6 +3200,11 @@ sql_create_index(struct Parse *parse, struct Token *token,
>  		 */
>  		pIndex->sort_order[i] = SORT_ORDER_ASC;
>  	}
> +
> +	set_index_def(parse, pIndex, pTab, zName,
> +		      nName, on_error, col_list,
> +		      idx_type);
> +

25. It fits in 2 lines.

> @@ -3133,6 +3270,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>  				}
>  				if (idx_type == SQLITE_IDXTYPE_PRIMARYKEY)
>  					pIdx->idxType = idx_type;
> +
>  				goto exit_create_index;
>  			}
>  		}

26. Garbage diff.

> @@ -3141,6 +3279,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>  	/* Link the new Index structure to its table and to the other
>  	 * in-memory database structures.
>  	 */
> +
>  	assert(parse->nErr == 0);
>  	if (db->init.busy) {
>  		Index *p;

27. Same.

28. index_column_count() is useless now.

29. index_is_unique_not_null() - same.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5fbcbaffc..34ac846cf 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -4298,7 +4298,7 @@ sqlite3IndexedByLookup(Parse * pParse, struct SrcList_item *pFrom)
>                 char *zIndexedBy = pFrom->u1.zIndexedBy;
>                 Index *pIdx;
>                 for (pIdx = pTab->pIndex;
> -                    pIdx && strcmp(pIdx->zName, zIndexedBy);
> +                    pIdx && strcmp(pIdx->def->name, zIndexedBy);
>                      pIdx = pIdx->pNext) ;
>                 if (!pIdx) {
>                         sqlite3ErrorMsg(pParse, "no such index: %s", zIndexedBy,

30. If you use def->name instead of zName, then remove zName from struct Index.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 950409d93..fe61dacfe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2124,6 +2124,7 @@ struct Index {
>  				 * or _NONE
>  				 */
>  	unsigned idxType:2;	/* 1==UNIQUE, 2==PRIMARY KEY, 0==CREATE INDEX */
> +    	struct index_def *def;
>  };

31. 4 spaces + 1 tab is bad indentation. Here 1 tab is enough.

> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 496b41725..6d95185b3 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -470,7 +470,7 @@ findIndexCol(Parse * pParse,	/* Parse context */
>  	for (int i = 0; i < pList->nExpr; i++) {
>  		Expr *p = sqlite3ExprSkipCollate(pList->a[i].pExpr);
>  		if (p->op == TK_COLUMN &&
> -		    p->iColumn == pIdx->aiColumn[iCol] &&
> +		    p->iColumn == (int) pIdx->def->key_def->parts[iCol].fieldno &&
>  		    p->iTable == iBase) {
>  			bool is_found;
>  			uint32_t id;

32. Out of 80.

> @@ -2859,6 +2859,19 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder,	/* WHERE clause information */
>  		sPk.aiRowLogEst = aiRowEstPk;
>  		sPk.onError = ON_CONFLICT_ACTION_REPLACE;
>  		sPk.pTable = pTab;
> +
> +		struct key_def *key_def = key_def_new(1);

33. Check on 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 index_opts = index_opts_default;
> +
> +		sPk.def =
> +			index_def_new(pTab->def->id, 0, "primary",
> +				      sizeof("primary") - 1, TREE,
> +				      &index_opts, key_def, NULL);

34. Same as 33. And it fits in 3 lines.

> diff --git a/test/box/suite.ini b/test/box/suite.ini
> index ddc71326b..94bc39c57 100644
> --- a/test/box/suite.ini
> +++ b/test/box/suite.ini
> @@ -2,7 +2,7 @@
>  core = tarantool
>  description = Database tests
>  script = box.lua
> -disabled = rtree_errinj.test.lua tuple_bench.test.lua
> +disabled = rtree_errinj.test.lua tuple_bench.test.lua errinj.test.lua
>  release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua
>  lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
>  use_unix_sockets = True

35. Why?


On 28/05/2018 11:51, Ivan Koptelov wrote: