[tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name
n.pettik
korablev at tarantool.org
Mon Oct 1 02:40:54 MSK 2018
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";')
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181001/31ae26f1/attachment.html>
More information about the Tarantool-patches
mailing list