Now, patch looks way better. Keep fixing minor remarks.Also, it is better to push your changes, so that I can checkTravis status (i.e. make sure that all tests have passed).On 4 Apr 2018, at 18:46, Hollow111 <hollow653@gmail.com> wrote:ср, 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.cindex 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 droppedNitpicking: put dot at the end of comments andstart sentences from capital letter.(This is just advice. You may skip most ’nitpicking’ comments.)Moreover, you forget to describe first arg:even despite the fact that it is obviously ’The parsing context'.*/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)You don’t need to place each arg at new line.The only restriction is 80 chars per line.{- 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) {It is better to use explicit == NULL check.+ 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.resultindex 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 snapshottest_run:grep_log("default", "saving snapshot", 400) == nil----- true+- false...test_run:grep_log("default", "making snapshot", 400) ~= nil----- true+- falseI guess, this diff doesn’t belong to the patch....-- restore default optionsbox.cfg{checkpoint_interval = 3600 * 4, checkpoint_count = 4 }