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 5CCE22ABDE for ; Wed, 5 Sep 2018 19:14:02 -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 7YY2GcwpxcBI for ; Wed, 5 Sep 2018 19:14:02 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 944112ABCE for ; Wed, 5 Sep 2018 19:14:01 -0400 (EDT) Content-Type: text/plain; charset=utf-8 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 From: "n.pettik" In-Reply-To: <1f1960f842e9443511b4bf2712a0b79bd7fb0764.1535711802.git.imeevma@gmail.com> Date: Thu, 6 Sep 2018 02:13:53 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <14AA0E00-B3ED-41E2-9E8D-575097320E05@tarantool.org> References: <1f1960f842e9443511b4bf2712a0b79bd7fb0764.1535711802.git.imeevma@gmail.com> 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 > 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 =3D = false }, > {{1, 'unsigned'}}} >=20 > - local stat1_ft =3D {{name=3D'tbl', type=3D'string'}, > - {name=3D'idx', type=3D'string'}, > + local stat1_ft =3D {{name=3D'tbl', type=3D'unsigned'}, > + {name=3D'idx', type=3D'unsigned=E2=80=99}, Nit: why you didn=E2=80=99t rename these fields to =E2=80=99space_id=E2=80= =99 and =E2=80=98index_id=E2=80=99 (like in _index)? The same for stat4. > @@ -793,8 +794,8 @@ analyzeOneTable(Parse * pParse, /* Parser = context */ > int regChng =3D iMem++; /* Index of changed index field */ > int regKey =3D iMem++; /* Key argument passed to stat_push() */ > int regTemp =3D iMem++; /* Temporary use register */ > - int regTabname =3D iMem++; /* Register containing table = name */ > - int regIdxname =3D iMem++; /* Register containing index = name */ > + int reg_space_id =3D iMem++; /* Register containing table id = */ > + int reg_index_id =3D 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 =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. > if (space_id =3D=3D BOX_ID_NIL) > return -1; > struct space *space =3D space_by_id(space_id); > assert(space !=3D NULL); > struct index *index; > - uint32_t iid =3D 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 !=3D BOX_ID_NIL) { > - index =3D space_index(space, iid); > - } else { > - if (sqlite3_stricmp(argv[0], argv[1]) !=3D 0) > - return -1; > - index =3D space_index(space, 0); > - } > + uint32_t iid =3D atoll(argv[1]); > + index =3D space_index(space, iid); > assert(index !=3D 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 =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) > + if (sqlite3_column_text(stmt, 0) =3D=3D NULL || > + sqlite3_column_text(stmt, 1) =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)); > + uint32_t space_id =3D sqlite3_column_int(stmt, 0); Why do you check on nullability text value, but fetch int? It looks suspicious=E2=80=A6 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 =3D select_copy; > } >=20 > -/** > - * Create expression record "@col_name =3D '@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 =3D parse->db; > - > - struct Expr *col_name_expr =3D sqlite3Expr(db, TK_ID, col_name); > - if (col_name_expr =3D=3D NULL) > - return NULL; > - struct Expr *col_value_expr =3D sqlite3Expr(db, TK_STRING, = col_value); > - if (col_value_expr =3D=3D 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? >=20 > /** > - * 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=E2=80=99t use this macros: it is awful and going to disappear. Just simply inline name of stat space. > } >=20 > /** > @@ -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); >=20 > exit_drop_table: > @@ -3119,11 +3089,12 @@ sql_drop_index(struct Parse *parse_context, = struct SrcList *index_name_list, > } >=20 > /* > - * 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 =3D require("sqltester") > -test:plan(121) > +test:plan(118) >=20 > -test:do_execsql_test( > - 15.7, > - [[ > - ANALYZE; > - UPDATE "_sql_stat1" SET "tbl" =3D '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=3D] > - - ['T1', 'T1', '1', '0', '0', !!binary kQE=3D] > - - ['T2', 'I1', '1', '0', '0', !!binary kQI=3D] > - - ['T2', 'T2', '1', '0', '0', !!binary kQE=3D] > +- - [0, '1', '0', '0', !!binary kQE=3D] > + - [1, '1', '0', '0', !!binary kQI=3D] > + - [0, '1', '0', '0', !!binary kQE=3D] > + - [1, '1', '0', '0', !!binary kQI=3D] > ... > -box.sql.execute("SELECT * FROM \"_sql_stat1\";") > +box.sql.execute('SELECT "idx","stat" FROM \"_sql_stat1\";=E2=80=99) Without space_id/space name this test is less representative. The same for other similar tests.