[tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index

[Hollow111 <hollow653@gmail.com>]

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

ср, 4 апр. 2018 г. в 17:06, n.pettik <korablev@tarantool.org>:

> Please, don’t hurry when working on patch.
> Consider carefully each fix and comment: there is no any deadline.
>
> You don’t have to send second patch version.
> This patch is pretty small, so you can answer to this letter and
> pin your changes. For example: you won’t change tests (since they are OK),
> so don’t include them in provided diff.
>
> > On 4 Apr 2018, at 00:37, 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/tree/N_Tatunov/gh-3264-stat-table-entries-removal
> > Issue: https://github.com/tarantool/tarantool/issues/3264
> >
> > src/box/sql/build.c                    |  41 ++++++-----
> > test/sql/sql-statN-index-drop.result   | 127
> +++++++++++++++++++++++++++++++++
> > test/sql/sql-statN-index-drop.test.lua |  54 ++++++++++++++
> > 3 files changed, 206 insertions(+), 16 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..44d7548 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -2207,25 +2207,34 @@ sqliteViewResetAll(sqlite3 * db)
> > #endif                                /* SQLITE_OMIT_VIEW */
> >
> > /*
> > - * Remove entries from the sqlite_statN tables (for N in (1,2,3))
> > + * Remove entries from the _sql_statN tables (for N in (1, 4))
> >  * after a DROP INDEX or DROP TABLE command.
> >  */
>
> Comment prior to function starts from /**.
> Your provide short description (it is already there),
> describe arguments and return value with @param and @retval tags.
> You can find more information by searching ‘doxygen comments’.
>
> > static void
> > -sqlite3ClearStatTables(Parse * pParse,       /* The parsing context */
> > -                    const char *zType,       /* "idx" or "tbl" */
> > -                    const char *zName        /* Name of index or table
> */
> > +sql_clear_stat_tables(Parse * pParse,        /* The parsing context */
>
> pParse and zType are examples of Hungarian notation:
> you don’t need ‘p’ and ‘z’ prefixes. Moreover, you can remove comment
> right after arguments, since args will be described above within oxygen
> comment.
>
> > +                    const char *zType,           /* "idx" or "tbl" */
> > +               const char *table_name,  /* Name of the table*/
> > +                    const char *idx_name     /* Name of the 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);
> > -             }
> > -     }
> > +    int i, j;
>
> You don’t need to declare vars beforehand:
> it is is required in obsolete C standards (such as KR or C89).
> However, we are using *modern* C99:
> for (int i = 0; …)  - is OK.
>
> > +    if(strcmp(zType, "idx") == 0)
>
> As you suggest, you can get rid of this ’type’. Instead, just
> check index name on nullability.
>
> > +        for(i = 1, j = 1; i <= 2; i++, j++) {
>
> This cycle-for is completely unreadable. Don’t make things to be
> complicated:
> it is better to use *less* beautiful, but more obvious and clear approach.
> Don’t be afraid of changing code: this function is declared as static,
> so it is not available for public usage.
>
> I would like to suggest you to get rid of cycle and snprintf,
> but add two almost the same calls of sqlite3NestedParse().
> It will result in plain and readable code:
> sqlite3NestedParse(…, ” … WHERE tbl = stat1”);
> sqlite3NestedParse(…, ” … WHERE tbl = stat4”);
>
> > +            char zTab[24];
> > +            sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * j);
> > +            sqlite3NestedParse(pParse,
> > +                        "DELETE FROM \"%s\" WHERE (\"idx\"=%Q AND "
> > +                        "\"tbl\"=%Q)",
> > +                        zTab, idx_name, table_name);
> > +        }
> > +    else
> > +        for(i = 1, j = 1; i <= 2; i++, j++) {
> > +            char zTab[24];
> > +            sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * j);
> > +            sqlite3NestedParse(pParse,
> > +                        "DELETE FROM \"%s\" WHERE \"tbl\"=%Q",
> > +                        zTab, table_name);
> > +        }
> > }
> >
> > /*
> > @@ -2415,7 +2424,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName,
> int isView, int noErr)
> >        */
> >
> >       sqlite3BeginWriteOperation(pParse, 1);
> > -     sqlite3ClearStatTables(pParse, "tbl", pTab->zName);
> > +    sql_clear_stat_tables(pParse, "tbl", pTab->zName, NULL);
> >       sqlite3FkDropTable(pParse, pName, pTab);
> >       sqlite3CodeDropTable(pParse, pTab, isView);
> >
> > @@ -3417,7 +3426,7 @@ sqlite3DropIndex(Parse * pParse, SrcList * pName,
> Token * pName2, int ifExists)
> >        * But firstly, delete statistics since schema
> >        * changes after DDL.
> >        */
> > -     sqlite3ClearStatTables(pParse, "idx", pIndex->zName);
> > +    sql_clear_stat_tables(pParse, "idx", pIndex->pTable->zName,
> pIndex->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..c7e476f
> > --- /dev/null
> > +++ b/test/sql/sql-statN-index-drop.result
> > @@ -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..bf4a752
> > --- /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.
> > +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.
> > +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
> >
> >
>
> Suggested changes were applied so this is diff:


diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 44d7548..16ae042 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2206,34 +2206,35 @@ sqliteViewResetAll(sqlite3 * db)
 #define sqliteViewResetAll(A,B)
 #endif /* SQLITE_OMIT_VIEW */

-/*
+/**
  * Remove entries from the _sql_statN tables (for N in (1, 4))
  * after a DROP INDEX or DROP TABLE command.
+ *
+ * @param table_name table to be dropped or
+ *                   the table that contains index to be dropped
+ * @param idx_name index to be dropped
  */
 static void
-sql_clear_stat_tables(Parse * pParse, /* The parsing context */
-        const char *zType,     /* "idx" or "tbl" */
-               const char *table_name,  /* Name of the table*/
-        const char *idx_name /* Name of the index*/
-    )
+sql_clear_stat_tables(Parse *parse,
+               const char *table_name,
+        const char *idx_name)
 {
-    int i, j;
-    if(strcmp(zType, "idx") == 0)
-        for(i = 1, j = 1; i <= 2; i++, j++) {
-            char zTab[24];
-            sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * j);
-            sqlite3NestedParse(pParse,
-                        "DELETE FROM \"%s\" WHERE (\"idx\"=%Q AND "
-                        "\"tbl\"=%Q)",
-                        zTab, idx_name, table_name);
-        }
-    else
-        for(i = 1, j = 1; i <= 2; i++, j++) {
-            char zTab[24];
-            sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * j);
-            sqlite3NestedParse(pParse,
-                        "DELETE FROM \"%s\" WHERE \"tbl\"=%Q",
-                        zTab, table_name);
+    if(idx_name) {
+        sqlite3NestedParse(parse,
+                    "DELETE FROM \"_sql_stat1\" WHERE (\"idx\"=%Q AND "
+                    "\"tbl\"=%Q)",
+                    idx_name, table_name);
+        sqlite3NestedParse(parse,
+                    "DELETE FROM \"_sql_stat4\" WHERE (\"idx\"=%Q AND "
+                    "\"tbl\"=%Q)",
+                    idx_name, table_name);
+    } else {
+        sqlite3NestedParse(parse,
+                    "DELETE FROM \"_sql_stat1\" WHERE \"tbl\"=%Q",
+                    table_name);
+        sqlite3NestedParse(parse,
+                    "DELETE FROM \"_sql_stat4\" WHERE \"tbl\"=%Q",
+                    table_name);
         }
 }

@@ -2424,7 +2425,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int
isView, int noErr)
  */

  sqlite3BeginWriteOperation(pParse, 1);
-    sql_clear_stat_tables(pParse, "tbl", pTab->zName, NULL);
+    sql_clear_stat_tables(pParse, pTab->zName, NULL);
  sqlite3FkDropTable(pParse, pName, pTab);
  sqlite3CodeDropTable(pParse, pTab, isView);

@@ -3426,7 +3427,7 @@ sqlite3DropIndex(Parse * pParse, SrcList * pName,
Token * pName2, int ifExists)
  * But firstly, delete statistics since schema
  * changes after DDL.
  */
-    sql_clear_stat_tables(pParse, "idx", pIndex->pTable->zName,
pIndex->zName);
+    sql_clear_stat_tables(pParse, pIndex->pTable->zName, pIndex->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/xlog/checkpoint_daemon.result
b/test/xlog/checkpoint_daemon.result
index d5ed666..1c28336 100644
--- a/test/xlog/checkpoint_daemon.result
+++ b/test/xlog/checkpoint_daemon.result
@@ -96,11 +96,11 @@ fiber.sleep(3 * PERIOD)
 -- check that it's not first snapshot
 test_run:grep_log("default", "saving snapshot", 400) == nil
 ---
-- true
+- false
 ...
 test_run:grep_log("default", "making snapshot", 400) ~= nil
 ---
-- true
+- false
 ...
 -- restore default options
 box.cfg{checkpoint_interval = 3600 * 4, checkpoint_count = 4 }

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