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

  • From: Hollow111 <hollow653@xxxxxxxxx>
  • To: korablev@xxxxxxxxxxxxx
  • Date: Wed, 04 Apr 2018 15:46:06 +0000

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

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@xxxxxxxxx> 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 }

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 04-2018