[tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name

[n.pettik]

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index d9c2ae4..bb8dc36 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -520,11 +520,11 @@ local function upgrade_to_2_1_0()
>     _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
>                   {{1, 'unsigned'}}}
> 
> -    local stat1_ft = {{name='tbl', type='string'},
> -                      {name='idx', type='string'},
> +    local stat1_ft = {{name='tbl', type='unsigned'},
> +                      {name='idx', type='unsigned’},

Nit: why you didn’t rename these fields to ’space_id’ and ‘index_id’
(like in _index)? The same for stat4.

> @@ -793,8 +794,8 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
> 	int regChng = iMem++;	/* Index of changed index field */
> 	int regKey = iMem++;	/* Key argument passed to stat_push() */
> 	int regTemp = iMem++;	/* Temporary use register */
> -	int regTabname = iMem++;	/* Register containing table name */
> -	int regIdxname = iMem++;	/* Register containing index name */
> +	int reg_space_id = iMem++;	/* Register containing table id */
> +	int reg_index_id = iMem++;	/* Register containing index id */

Nit: put comments above the code to be commented and put dots
at the end of comment.

> @@ -1246,24 +1246,14 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
> 	struct analysis_index_info *info = (struct analysis_index_info *) data;
> 	assert(info->stats != NULL);
> 	struct index_stat *stat = &info->stats[info->index_count++];
> -	uint32_t space_id = box_space_id_by_name(argv[0], strlen(argv[0]));
> +	uint32_t space_id = atoll(argv[0]);

I would add assertion like:
assert(space_id != 0);
Since atoll in case of fail returns 0 and BOX_ID_NIL != 0.

> 	if (space_id == BOX_ID_NIL)
> 		return -1;
> 	struct space *space = space_by_id(space_id);
> 	assert(space != NULL);
> 	struct index *index;
> -	uint32_t iid = box_index_id_by_name(space_id, argv[1], strlen(argv[1]));
> -	/*
> -	 * Convention is if index's name matches with space's
> -	 * one, then it is primary index.
> -	 */
> -	if (iid != BOX_ID_NIL) {
> -		index = space_index(space, iid);
> -	} else {
> -		if (sqlite3_stricmp(argv[0], argv[1]) != 0)
> -			return -1;
> -		index = space_index(space, 0);
> -	}
> +	uint32_t iid = atoll(argv[1]);
> +	index = space_index(space, iid);
> 	assert(index != NULL);
> 	/*
> 	 * Additional field is used to describe total
> @@ -1410,27 +1400,17 @@ load_stat_from_space(struct sqlite3 *db, const char *sql_select_prepare,
> 		goto finalize;
> 	uint32_t current_idx_count = 0;
> 	while (sqlite3_step(stmt) == SQLITE_ROW) {
> -		const char *space_name = (char *)sqlite3_column_text(stmt, 0);
> -		if (space_name == NULL)
> -			continue;
> -		const char *index_name = (char *)sqlite3_column_text(stmt, 1);
> -		if (index_name == NULL)
> +		if (sqlite3_column_text(stmt, 0) == NULL ||
> +		    sqlite3_column_text(stmt, 1) == NULL)
> 			continue;
> -		uint32_t sample_count = sqlite3_column_int(stmt, 2);
> -		uint32_t space_id = box_space_id_by_name(space_name,
> -							 strlen(space_name));
> +		uint32_t space_id = sqlite3_column_int(stmt, 0);

Why do you check on nullability text value, but fetch int?
It looks suspicious…
The same for other usages.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a1e16b2..ece1092 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1888,75 +1888,45 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
> 	parse_context->parsed_ast.select = select_copy;
> }
> 
> -/**
> - * Create expression record "@col_name = '@col_value'".
> - *
> - * @param parse The parsing context.
> - * @param col_name Name of column.
> - * @param col_value Name of row.
> - * @retval not NULL on success.
> - * @retval NULL on failure.
> - */
> -static struct Expr *
> -sql_id_eq_str_expr(struct Parse *parse, const char *col_name,
> -		   const char *col_value)
> -{
> -	struct sqlite3 *db = parse->db;
> -
> -	struct Expr *col_name_expr = sqlite3Expr(db, TK_ID, col_name);
> -	if (col_name_expr == NULL)
> -		return NULL;
> -	struct Expr *col_value_expr = sqlite3Expr(db, TK_STRING, col_value);
> -	if (col_value_expr == NULL) {
> -		sql_expr_delete(db, col_name_expr, false);
> -		return NULL;
> -	}
> -	return sqlite3PExpr(parse, TK_EQ, col_name_expr, col_value_expr);
> -}
> -
> void
> -vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> -			   const char *idx_name, const char *table_name)
> +sql_remove_from_stat(struct Parse *parse, const char *stat_table_name,
> +		     uint32_t space_id, uint32_t index_id)

Remove what? Why did you rename function?

> 
> /**
> - * Remove entries from the _sql_stat1 and _sql_stat4
> - * system spaces after a DROP INDEX or DROP TABLE command.
> + * Remove entries from the _sql_stat1 and _sql_stat4 system spaces
> + * after a DROP INDEX or DROP TABLE command.

Nit: noise diff.

>  *
> - * @param parse      The parsing context.
> - * @param table_name The table to be dropped or
> - *                   the table that contains index to be dropped.
> - * @param idx_name   Index to be dropped.
> + * @param parse The parsing context.
> + * @param table_name The table to be dropped or the table that
> + *        contains index to be dropped.

You forgot to rename args here.

> + * @param idx_name Index to be dropped.
>  */
> static void
> -sql_clear_stat_spaces(struct Parse *parse, const char *table_name,
> -		      const char *idx_name)
> +sql_clear_stat_spaces(struct Parse *parse, uint32_t space_id, uint32_t index_id)
> {
> -	vdbe_emit_stat_space_clear(parse, "_sql_stat4", idx_name, table_name);
> -	vdbe_emit_stat_space_clear(parse, "_sql_stat1", idx_name, table_name);
> +	sql_remove_from_stat(parse, TARANTOOL_SYS_SQL_STAT1_NAME, space_id,
> +			     index_id);
> +	sql_remove_from_stat(parse, TARANTOOL_SYS_SQL_STAT4_NAME, space_id,
> +			     index_id);

Don’t use this macros: it is awful and going to disappear.
Just simply inline name of stat space.

> }
> 
> /**
> @@ -2177,7 +2147,7 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
> 			goto exit_drop_table;
> 		}
> 	}
> -	sql_clear_stat_spaces(parse_context, space_name, NULL);
> +	sql_clear_stat_spaces(parse_context, space->def->id, BOX_ID_NIL);
> 	sql_code_drop_table(parse_context, space, is_view);
> 
>  exit_drop_table:
> @@ -3119,11 +3089,12 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
> 	}
> 
> 	/*
> -	 * Generate code to remove entry from _index space
> -	 * But firstly, delete statistics since schema
> -	 * changes after DDL.
> +	 * Generate code to remove entry from _index space.
> +	 * But firstly, delete statistics since schema changes
> +	 * after DDL.

Nit: again noise diff.

> diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
> index 1dbfe5d..2c6b4b8 100755
> --- a/test/sql-tap/analyze9.test.lua
> +++ b/test/sql-tap/analyze9.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(121)
> +test:plan(118)
> 
> -test:do_execsql_test(
> -    15.7,
> -    [[
> -        ANALYZE;
> -        UPDATE "_sql_stat1" SET "tbl" = 'no such tbl';
> -    ]])

Why did you simply delete this test? Rewrite it pls using space id.

> diff --git a/test/sql/sql-statN-index-drop.result b/test/sql/sql-statN-index-drop.result
> index a751eca..0e5f2a3 100644
> --- a/test/sql/sql-statN-index-drop.result
> +++ b/test/sql/sql-statN-index-drop.result
> @@ -31,36 +31,36 @@ box.sql.execute("ANALYZE;")
> ---
> ...
> -- Checking the data.
> -box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> +box.sql.execute('SELECT "idx","neq","nlt","ndlt","sample" FROM \"_sql_stat4\";')
> ---
> -- - ['T1', 'I1', '1', '0', '0', !!binary kQI=]
> -  - ['T1', 'T1', '1', '0', '0', !!binary kQE=]
> -  - ['T2', 'I1', '1', '0', '0', !!binary kQI=]
> -  - ['T2', 'T2', '1', '0', '0', !!binary kQE=]
> +- - [0, '1', '0', '0', !!binary kQE=]
> +  - [1, '1', '0', '0', !!binary kQI=]
> +  - [0, '1', '0', '0', !!binary kQE=]
> +  - [1, '1', '0', '0', !!binary kQI=]
> ...
> -box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +box.sql.execute('SELECT "idx","stat" FROM \"_sql_stat1\";’)

Without space_id/space name this test is less representative.
The same for other similar tests.