ср, 4 апр. 2018 г. в 19:11, n.pettik : > Now, patch looks way better. Keep fixing minor remarks. > Also, it is better to push your changes, so that I can check > Travis status (i.e. make sure that all tests have passed). > > > On 4 Apr 2018, at 18:46, Hollow111 wrote: > > > > ср, 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 > > > Nitpicking: put dot at the end of comments and > start 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.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 > > > I guess, this diff doesn’t belong to the patch. > > ... > -- restore default options > box.cfg{checkpoint_interval = 3600 * 4, checkpoint_count = 4 } > > > > Diff after the changes: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 16ae042..958927e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2210,16 +2210,17 @@ sqliteViewResetAll(sqlite3 * db) * 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 + * @param parse The parsing context. + * @param table_name The 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 *parse, - const char *table_name, - const char *idx_name) +sql_clear_stat_tables(Parse *parse, const char *table_name, + const char *idx_name + ) { - if(idx_name) { + if(idx_name != NULL) { sqlite3NestedParse(parse, "DELETE FROM \"_sql_stat1\" WHERE (\"idx\"=%Q AND " "\"tbl\"=%Q)",