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

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

[-- Attachment #1: Type: text/plain, Size: 6700 bytes --]

Sorry for quite late respond, I sincerely regret it.

Patch LGTM (except for several minor nits).

> diff --git a/src/box/sql.c b/src/box/sql.c
> index b158c50..87f2088 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1217,21 +1217,21 @@ void tarantoolSqlite3LoadSchema(struct init_data *init)
>      sql_init_callback(init, TARANTOOL_SYS_SQL_STAT1_NAME,
>                BOX_SQL_STAT1_ID, 0,
>                "CREATE TABLE \""TARANTOOL_SYS_SQL_STAT1_NAME
> -                   "\"(\"tbl\" text,"
> -                   "\"idx\" text,"
> +                   "\"(\"space_id\" INT,"
> +                   "\"index_id\" INT,"
>                     "\"stat\" not null,"
> -                   "PRIMARY KEY(\"tbl\", \"idx\"))");
> +                   "PRIMARY KEY(\"space_id\", \"index_id\"))");
> 
>      sql_init_callback(init, TARANTOOL_SYS_SQL_STAT4_NAME,
>                BOX_SQL_STAT4_ID, 0,
>                "CREATE TABLE \""TARANTOOL_SYS_SQL_STAT4_NAME
> -                   "\"(\"tbl\" text,"
> -                   "\"idx\" text,"
> +                   "\"(\"space_id\" INT,"
> +                   "\"index_id\" INT,"
>                     "\"neq\" text,"
>                     "\"nlt\" text,"
>                     "\"ndlt\" text,"
>                     "\"sample\","
> -                   "PRIMARY KEY(\"tbl\", \"idx\", \"sample\"))");
> +                   "PRIMARY KEY(\"space_id\", \"index_id\", \"sample\"))”);

Note: after you rebase on fresh 2.0, these fixes will disappear, since
we don’t add system spaces to separate hash anymore.

> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 76ae153..6938dcb 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -38,8 +38,8 @@
>   *
>   * The following system tables are or have been supported:
>   *
> - *    CREATE TABLE _sql_stat1(tbl, idx, stat);
> - *    CREATE TABLE _sql_stat4(tbl, idx, nEq, nLt, nDLt, sample);
> + *    CREATE TABLE _sql_stat1(space_id, index_id, stat);
> + *    CREATE TABLE _sql_stat4(space_id, index_id, nEq, nLt, nDLt, sample);
>   *
>   * For most applications, _sql_stat1 provides all the statistics required
>   * for the query planner to make good choices.
> @@ -47,7 +47,7 @@
>   * Format of _sql_stat1:
>   *
>   * There is normally one row per index, with the index identified by the
> - * name in the idx column.  The tbl column is the name of the table to
> + * name in the index_id column.  The space_id column is the name of the table to

Nit:  * id in the index_id column. The space_id column is the id of the space to

> @@ -1410,27 +1405,14 @@ 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)
> -            continue;
> -        uint32_t sample_count = sqlite3_column_int(stmt, 2);
> -        uint32_t space_id = box_space_id_by_name(space_name,
> -                             strlen(space_name));
> -        assert(space_id != BOX_ID_NIL);
> +        uint32_t space_id = sqlite3_column_int(stmt, 0);
> +        assert(space_id != BOX_ID_NIL && space_id != 0);

Above you have mentioned that space_id now can’t be equal to BOX_ID_NIL:

>>> @@ -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.
> Actually now we cannot get BOX_ID_NIL, but we can get 0. Changed "if"
> that placed next line after "atoll”.

So, why do you check within assertion that condition? I mean there is no crime,
but does it make sense? The same for other similar checks.

> --- a/test/sql/sql-statN-index-drop.test.lua
> +++ b/test/sql/sql-statN-index-drop.test.lua
> @@ -14,15 +14,15 @@ box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
>  box.sql.execute("ANALYZE;")
> 
>  -- Checking the data.
> -box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> -box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
> +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";’)

Nit: I don’t think that in this test (and the rest below) we need to fetch
name of space instead of its id. Lets leave this test as it was before.

> 
>  -- Dropping an index.
>  box.sql.execute("DROP INDEX i1 ON t1;")
> 
>  -- Checking the DROP INDEX results.
> -box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> -box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
> +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')
> 
>  --Cleaning up.
>  box.sql.execute("DROP TABLE t1;")
> @@ -41,15 +41,15 @@ box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
>  box.sql.execute("ANALYZE;")
> 
>  -- Checking the data.
> -box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> -box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
> +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')
> 
>  -- Dropping an index.
>  box.sql.execute("DROP INDEX i1 ON t2;")
> 
>  -- Checking the DROP INDEX results.
> -box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> -box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +box.sql.execute('SELECT "name","index_id","neq","nlt","ndlt","sample" FROM "_sql_stat4" join "_space" on "_sql_stat4"."space_id" = "_space"."id";')
> +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" join "_space" on "_sql_stat1"."space_id" = "_space"."id";')
> 


[-- Attachment #2: Type: text/html, Size: 72555 bytes --]