From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9BBD72B18E for ; Sun, 30 Sep 2018 19:41:01 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id khKm66pk9Opg for ; Sun, 30 Sep 2018 19:41:01 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5D8742B099 for ; Sun, 30 Sep 2018 19:41:00 -0400 (EDT) From: "n.pettik" Message-Id: <70A5C3C7-8069-4FF0-A653-46B3B82C9716@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_20528C93-B1DD-4597-A35E-3ED59F4CC352" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: hold in stat tables space/index id instead of name Date: Mon, 1 Oct 2018 02:40:54 +0300 In-Reply-To: <0bbf0c18-9d9b-4849-5a01-24729ab08468@tarantool.org> References: <1f1960f842e9443511b4bf2712a0b79bd7fb0764.1535711802.git.imeevma@gmail.com> <14AA0E00-B3ED-41E2-9E8D-575097320E05@tarantool.org> <0bbf0c18-9d9b-4849-5a01-24729ab08468@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Imeev Mergen --Apple-Mail=_20528C93-B1DD-4597-A35E-3ED59F4CC352 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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\"))"); >=20 > 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\"))=E2=80=9D); Note: after you rebase on fresh 2.0, these fixes will disappear, since we don=E2=80=99t 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 =3D 0; > while (sqlite3_step(stmt) =3D=3D SQLITE_ROW) { > - const char *space_name =3D (char *)sqlite3_column_text(stmt, = 0); > - if (space_name =3D=3D NULL) > - continue; > - const char *index_name =3D (char *)sqlite3_column_text(stmt, = 1); > - if (index_name =3D=3D NULL) > - continue; > - uint32_t sample_count =3D sqlite3_column_int(stmt, 2); > - uint32_t space_id =3D box_space_id_by_name(space_name, > - strlen(space_name)); > - assert(space_id !=3D BOX_ID_NIL); > + uint32_t space_id =3D sqlite3_column_int(stmt, 0); > + assert(space_id !=3D BOX_ID_NIL && space_id !=3D 0); Above you have mentioned that space_id now can=E2=80=99t 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 =3D (struct analysis_index_info = *) data; >>> assert(info->stats !=3D NULL); >>> struct index_stat *stat =3D &info->stats[info->index_count++]; >>> - uint32_t space_id =3D box_space_id_by_name(argv[0], = strlen(argv[0])); >>> + uint32_t space_id =3D atoll(argv[0]); >> I would add assertion like: >> assert(space_id !=3D 0); >> Since atoll in case of fail returns 0 and BOX_ID_NIL !=3D 0. > Actually now we cannot get BOX_ID_NIL, but we can get 0. Changed "if" > that placed next line after "atoll=E2=80=9D. 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;") >=20 > -- 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" =3D = "_space"."id";') > +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" = join "_space" on "_sql_stat1"."space_id" =3D "_space"."id";=E2=80=99) Nit: I don=E2=80=99t 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. >=20 > -- Dropping an index. > box.sql.execute("DROP INDEX i1 ON t1;") >=20 > -- 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" =3D = "_space"."id";') > +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" = join "_space" on "_sql_stat1"."space_id" =3D "_space"."id";') >=20 > --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;") >=20 > -- 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" =3D = "_space"."id";') > +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" = join "_space" on "_sql_stat1"."space_id" =3D "_space"."id";') >=20 > -- Dropping an index. > box.sql.execute("DROP INDEX i1 ON t2;") >=20 > -- 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" =3D = "_space"."id";') > +box.sql.execute('SELECT "name","index_id","stat" FROM "_sql_stat1" = join "_space" on "_sql_stat1"."space_id" =3D "_space"."id";') >=20 --Apple-Mail=_20528C93-B1DD-4597-A35E-3ED59F4CC352 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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\"))=E2=80=9D);

Note: after = you rebase on fresh 2.0, these fixes will disappear, since
we = don=E2=80=99t 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 =3D 0;
     while = (sqlite3_step(stmt) =3D=3D SQLITE_ROW) {
-    =     const char *space_name =3D (char = *)sqlite3_column_text(stmt, 0);
-    =     if (space_name =3D=3D NULL)
-        =     continue;
-    =     const char *index_name =3D (char = *)sqlite3_column_text(stmt, 1);
-    =     if (index_name =3D=3D NULL)
-        =     continue;
-    =     uint32_t sample_count =3D sqlite3_column_int(stmt, = 2);
-        = uint32_t space_id =3D box_space_id_by_name(space_name,
-        =             =         =  strlen(space_name));
-    =     assert(space_id !=3D BOX_ID_NIL);
+        uint32_t = space_id =3D sqlite3_column_int(stmt, 0);
+    =     assert(space_id !=3D BOX_ID_NIL && space_id = !=3D 0);

Above you have mentioned that space_id now can=E2=80= =99t 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 = =3D (struct analysis_index_info *) data;
= assert(info->stats !=3D NULL);
struct = index_stat *stat =3D &info->stats[info->index_count++];
- = uint32_t space_id =3D box_space_id_by_name(argv[0], = strlen(argv[0]));
+ uint32_t space_id =3D = atoll(argv[0]);
I would add assertion = like:
assert(space_id !=3D 0);
Since atoll = in case of fail returns 0 and BOX_ID_NIL !=3D 0.
Actually now we cannot get BOX_ID_NIL, but we can = get 0. Changed "if"
that placed next line after = "atoll=E2=80=9D.

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" =3D "_space"."id";')
+box.sql.execute('SELECT = "name","index_id","stat" FROM "_sql_stat1" join "_space" on = "_sql_stat1"."space_id" =3D "_space"."id";=E2=80=99)

Nit: I don=E2=80=99= 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" =3D "_space"."id";')
+box.sql.execute('SELECT = "name","index_id","stat" FROM "_sql_stat1" join "_space" on = "_sql_stat1"."space_id" =3D "_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" =3D "_space"."id";')
+box.sql.execute('SELECT = "name","index_id","stat" FROM "_sql_stat1" join "_space" on = "_sql_stat1"."space_id" =3D "_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" =3D "_space"."id";')
+box.sql.execute('SELECT = "name","index_id","stat" FROM "_sql_stat1" join "_space" on = "_sql_stat1"."space_id" =3D "_space"."id";')

= --Apple-Mail=_20528C93-B1DD-4597-A35E-3ED59F4CC352--