ср, 4 апр. 2018 г. в 17:06, n.pettik : > 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 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 }