* [tarantool-patches] Re: [PATCH] sql: statistics removal after dropping an index
2018-04-03 18:07 [tarantool-patches] [PATCH] sql: statistics removal after dropping an index N.Tatunov
@ 2018-04-03 19:40 ` n.pettik
0 siblings, 0 replies; 2+ messages in thread
From: n.pettik @ 2018-04-03 19:40 UTC (permalink / raw)
To: tarantool-patches; +Cc: N. Tatunov
[-- Attachment #1: Type: text/plain, Size: 10992 bytes --]
Hello. Consider comments below.
> On 3 Apr 2018, at 21:07, N.Tatunov <hollow653@gmail.com> wrote:
>
> Currently dropping an index leads to removal of
> all the entries containing the certain index name
> in "_sql_statN" tables. Thus far analyze routine was fixed
> so it seems that the indexes from the different tables but
> with the same names should work more properly.
>
> Closes: #3264
> ---
>
> Branch: https://github.com/tarantool/tarantool
This is link to the whole repo.
You should place here link to your particular branch.
Example:
https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3264-stat-table-entries-removal <https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3264-stat-table-entries-removal>
> Issue: https://github.com/tarantool/tarantool/issues/3264
>
> src/box/sql/build.c | 36 ++++++----
> test/sql/sql-statN-index-drop.result | 127 +++++++++++++++++++++++++++++++++
> test/sql/sql-statN-index-drop.test.lua | 54 ++++++++++++++
> 3 files changed, 205 insertions(+), 12 deletions(-)
> create mode 100644 test/sql/sql-statN-index-drop.result
> create mode 100644 test/sql/sql-statN-index-drop.test.lua
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 5e3ed0f..baaade2 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2213,19 +2213,31 @@ sqliteViewResetAll(sqlite3 * db)
> static void
> sqlite3ClearStatTables(Parse * pParse, /* The parsing context */
> const char *zType, /* "idx" or "tbl" */
> - const char *zName /* Name of index or table */
> + const char *zName, /* Name of index or table */
> + const char *zTable /* Name of the indexed table*/
> )
Way you has chosen is very confusing.
Here table name is passed as third arg and index is NULL:
> + sqlite3ClearStatTables(pParse, "tbl", pTab->zName, NULL);
But here - vice versa:
> + sqlite3ClearStatTables(pParse, "idx", pIndex->zName, pIndex->pTable->zName);
Lets strictly specify, that third arg always is a table's name,
and fourth one - index.
> {
> int i;
> - for (i = 1; i <= 4; i++) {
> - char zTab[24];
> - sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i);
> - if (sqlite3FindTable(pParse->db, zTab)) {
> - sqlite3NestedParse(pParse,
> - "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
> - zTab, zType, zName);
> - }
> - }
> + if(strcmp(zType, "idx") == 0)
> + for (i = 1; i <= 4; i++) {
Actually, it is legacy code: it derived from SQLite codebase.
In SQLite really can exist all four[1-4] tables. But in our SQL
only _stat1 and _stat4 tables are valid tables.
> + char zTab[24];
> + sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i);
> + if (sqlite3FindTable(pParse->db, zTab)) {
Statistic tables are considered to be system.
This means that they are created when Tarantool is launching.
Thus, they always exist and you don’t need to check if table exists.
In this case existence is checked due to the fact that tables
_stat2 and _stat3 does not exist. So I propose you to involve only
required tables and get rid of this check.
> + sqlite3NestedParse(pParse,
> + "DELETE FROM \"%s\" WHERE (\"%s\"=%Q AND "
> + "\"tbl\"=%Q)",
> + zTab, zType, zName, zTable);
> + }
> + } else
> + for (i = 1; i <= 4; i++) {
> + char zTab[24];
> + sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i);
> + if (sqlite3FindTable(pParse->db, zTab)) {
> + sqlite3NestedParse(pParse,
> + "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
> + zTab, zType, zName);
> + }
> + }
> }
Overall comment: you may notice that now SQL codebase is written using
legacy codestyle (including obsolete var declaration style and
Hungarian notation). I suggest you to rewrite whole function using
Tarantool (aka Linux Kernel) codestyle and eventually rename it to
'sql_clear_stat_tables'. Also, we are using comments for functions
in doxygen format. You can check it out in any src/box source file.
>
> /*
> @@ -2415,7 +2427,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr)
> */
>
> sqlite3BeginWriteOperation(pParse, 1);
> - sqlite3ClearStatTables(pParse, "tbl", pTab->zName);
> + sqlite3ClearStatTables(pParse, "tbl", pTab->zName, NULL);
> sqlite3FkDropTable(pParse, pName, pTab);
> sqlite3CodeDropTable(pParse, pTab, isView);
>
> @@ -3417,7 +3429,7 @@ sqlite3DropIndex(Parse * pParse, SrcList * pName, Token * pName2, int ifExists)
> * But firstly, delete statistics since schema
> * changes after DDL.
> */
> - sqlite3ClearStatTables(pParse, "idx", pIndex->zName);
> + sqlite3ClearStatTables(pParse, "idx", pIndex->zName, pIndex->pTable->zName);
> int record_reg = ++pParse->nMem;
> int space_id_reg = ++pParse->nMem;
> sqlite3VdbeAddOp2(v, OP_Integer, SQLITE_PAGENO_TO_SPACEID(pIndex->tnum),
> diff --git a/test/sql/sql-statN-index-drop.result b/test/sql/sql-statN-index-drop.result
> new file mode 100644
> index 0000000..707d7e0
> --- /dev/null
> +++ b/test/sql/sql-statN-index-drop.result
Actually, you don’t have to create separate test file for this issue:
you can add these tests to one of sql-tap/analyze[1-11].test.lua
But it is up to you.
> @@ -0,0 +1,127 @@
> +test_run = require('test_run').new()
> +---
> +...
> +-- Initializing some things.
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a);")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(id PRIMARY KEY, a);")
> +---
> +...
> +box.sql.execute("CREATE INDEX i1 ON t1(a);")
> +---
> +...
> +box.sql.execute("CREATE INDEX i1 ON t2(a);")
> +---
> +...
> +box.sql.execute("INSERT INTO t1 VALUES(1, 2);")
> +---
> +...
> +box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
> +---
> +...
> +-- Analyze.
> +box.sql.execute("ANALYZE;")
> +---
> +...
> +-- Checking the data.
> +box.sql.execute("SELECT * 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=]
> +...
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +---
> +- - ['T1', 'I1', '1 1']
> + - ['T1', 'T1', '1 1']
> + - ['T2', 'I1', '1 1']
> + - ['T2', 'T2', '1 1']
> +...
> +-- Dropping an index
> +box.sql.execute("DROP INDEX i1 ON t1;")
> +---
> +...
> +-- Checking the DROP INDEX results.
> +box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> +---
> +- - ['T1', 'T1', '1', '0', '0', !!binary kQE=]
> + - ['T2', 'I1', '1', '0', '0', !!binary kQI=]
> + - ['T2', 'T2', '1', '0', '0', !!binary kQE=]
> +...
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +---
> +- - ['T1', 'T1', '1 1']
> + - ['T2', 'I1', '1 1']
> + - ['T2', 'T2', '1 1']
> +...
> +--Cleaning up.
> +box.sql.execute("DROP TABLE t1;")
> +---
> +...
> +box.sql.execute("DROP TABLE t2;")
> +---
> +...
> +-- Same test but dropping an INDEX ON t2.
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a);")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(id PRIMARY KEY, a);")
> +---
> +...
> +box.sql.execute("CREATE INDEX i1 ON t1(a);")
> +---
> +...
> +box.sql.execute("CREATE INDEX i1 ON t2(a);")
> +---
> +...
> +box.sql.execute("INSERT INTO t1 VALUES(1, 2);")
> +---
> +...
> +box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
> +---
> +...
> +-- Analyze.
> +box.sql.execute("ANALYZE;")
> +---
> +...
> +-- Checking the data.
> +box.sql.execute("SELECT * 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=]
> +...
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +---
> +- - ['T1', 'I1', '1 1']
> + - ['T1', 'T1', '1 1']
> + - ['T2', 'I1', '1 1']
> + - ['T2', 'T2', '1 1']
> +...
> +-- Dropping an index
> +box.sql.execute("DROP INDEX i1 ON t2;")
> +---
> +...
> +-- Checking the DROP INDEX results.
> +box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> +---
> +- - ['T1', 'I1', '1', '0', '0', !!binary kQI=]
> + - ['T1', 'T1', '1', '0', '0', !!binary kQE=]
> + - ['T2', 'T2', '1', '0', '0', !!binary kQE=]
> +...
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +---
> +- - ['T1', 'I1', '1 1']
> + - ['T1', 'T1', '1 1']
> + - ['T2', 'T2', '1 1']
> +...
> +--Cleaning up.
> +box.sql.execute("DROP TABLE t1;")
> +---
> +...
> +box.sql.execute("DROP TABLE t2;")
> +---
> +...
> diff --git a/test/sql/sql-statN-index-drop.test.lua b/test/sql/sql-statN-index-drop.test.lua
> new file mode 100644
> index 0000000..9b919de
> --- /dev/null
> +++ b/test/sql/sql-statN-index-drop.test.lua
> @@ -0,0 +1,54 @@
> +test_run = require('test_run').new()
> +
> +-- Initializing some things.
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a);")
> +box.sql.execute("CREATE TABLE t2(id PRIMARY KEY, a);")
> +box.sql.execute("CREATE INDEX i1 ON t1(a);")
> +box.sql.execute("CREATE INDEX i1 ON t2(a);")
> +box.sql.execute("INSERT INTO t1 VALUES(1, 2);")
> +box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
> +
> +-- Analyze.
> +box.sql.execute("ANALYZE;")
> +
> +-- Checking the data.
> +box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +
> +-- Dropping an index
Nitpicking: put a dot at the end of sentence.
> +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\";")
> +
> +--Cleaning up.
> +box.sql.execute("DROP TABLE t1;")
> +box.sql.execute("DROP TABLE t2;")
> +
> +-- Same test but dropping an INDEX ON t2.
> +
> +box.sql.execute("CREATE TABLE t1(id PRIMARY KEY, a);")
> +box.sql.execute("CREATE TABLE t2(id PRIMARY KEY, a);")
> +box.sql.execute("CREATE INDEX i1 ON t1(a);")
> +box.sql.execute("CREATE INDEX i1 ON t2(a);")
> +box.sql.execute("INSERT INTO t1 VALUES(1, 2);")
> +box.sql.execute("INSERT INTO t2 VALUES(1, 2);")
> +
> +-- Analyze.
> +box.sql.execute("ANALYZE;")
> +
> +-- Checking the data.
> +box.sql.execute("SELECT * FROM \"_sql_stat4\";")
> +box.sql.execute("SELECT * FROM \"_sql_stat1\";")
> +
> +-- Dropping an index
Nitpicking: put a dot at the end of sentence.
> +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\";")
> +
> +--Cleaning up.
> +box.sql.execute("DROP TABLE t1;")
> +box.sql.execute("DROP TABLE t2;")
> --
> 2.7.4
>
>
[-- Attachment #2: Type: text/html, Size: 20504 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread