* [tarantool-patches] [PATCH 2/2] sql: statistics removal after dropping an index
@ 2018-04-03 21:37 N.Tatunov
2018-04-04 14:06 ` [tarantool-patches] " n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: N.Tatunov @ 2018-04-03 21:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: korablev, N.Tatunov
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.
*/
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 */
+ 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;
+ 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);
+ }
}
/*
@@ -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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-03 21:37 [tarantool-patches] [PATCH 2/2] sql: statistics removal after dropping an index N.Tatunov
@ 2018-04-04 14:06 ` n.pettik
2018-04-04 15:46 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-04 14:06 UTC (permalink / raw)
To: N. Tatunov; +Cc: tarantool-patches
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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-04 14:06 ` [tarantool-patches] " n.pettik
@ 2018-04-04 15:46 ` Hollow111
2018-04-04 16:11 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-04 15:46 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-04 15:46 ` Hollow111
@ 2018-04-04 16:11 ` n.pettik
2018-04-04 16:34 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-04 16:11 UTC (permalink / raw)
To: Hollow111; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 17108 bytes --]
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 <hollow653@gmail.com> wrote:
>
>
>
> ср, 4 апр. 2018 г. в 17:06, n.pettik <korablev@tarantool.org <mailto: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 <mailto: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 <https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3264-stat-table-entries-removal>
> > Issue: https://github.com/tarantool/tarantool/issues/3264 <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 }
>
[-- Attachment #2: Type: text/html, Size: 29418 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-04 16:11 ` n.pettik
@ 2018-04-04 16:34 ` Hollow111
2018-04-05 18:01 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-04 16:34 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 18549 bytes --]
ср, 4 апр. 2018 г. в 19:11, n.pettik <korablev@tarantool.org>:
> 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 <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.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)",
[-- Attachment #2: Type: text/html, Size: 26287 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-04 16:34 ` Hollow111
@ 2018-04-05 18:01 ` n.pettik
[not found] ` <CAEi+_aq5oyeB0cbnxAXXjQqu=h+PCGaaZuLkk3p33yq371+Xog@mail.gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-05 18:01 UTC (permalink / raw)
To: N. Tatunov; +Cc: tarantool-patches
Hello.
See 2 minor remarks. The rest seems to be OK.
I have noticed, that there are wrong indentations on your branch:
@@ -2415,7 +2426,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr)
*/
sqlite3BeginWriteOperation(pParse, 1);
- sqlite3ClearStatTables(pParse, "tbl", pTab->zName);
+ sql_clear_stat_tables(pParse, pTab->zName, NULL);
@@ -3417,7 +3428,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, pIndex->pTable->zName, pIndex->zName);
Please, fix them.
> On 4 Apr 2018, at 19:34, Hollow111 <hollow653@gmail.com> wrote:
>
> @@ -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
> + )
This bracket should be on previous line.
> {
> - if(idx_name) {
> + if(idx_name != NULL) {
> sqlite3NestedParse(parse,
> "DELETE FROM \"_sql_stat1\" WHERE (\"idx\"=%Q AND "
> "\"tbl\"=%Q)",
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
[not found] ` <CAEi+_aq5oyeB0cbnxAXXjQqu=h+PCGaaZuLkk3p33yq371+Xog@mail.gmail.com>
@ 2018-04-07 2:12 ` Hollow111
2018-04-09 12:16 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-07 2:12 UTC (permalink / raw)
To: korablev, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 5280 bytes --]
пт, 6 апр. 2018 г. в 3:42, Hollow111 <hollow653@gmail.com>:
>
>
> чт, 5 апр. 2018 г. в 21:01, n.pettik <korablev@tarantool.org>:
>
>> Hello.
>>
>> See 2 minor remarks. The rest seems to be OK.
>>
>> I have noticed, that there are wrong indentations on your branch:
>>
>> @@ -2415,7 +2426,7 @@ sqlite3DropTable(Parse * pParse, SrcList * pName,
>> int isView, int noErr)
>> */
>>
>> sqlite3BeginWriteOperation(pParse, 1);
>> - sqlite3ClearStatTables(pParse, "tbl", pTab->zName);
>> + sql_clear_stat_tables(pParse, pTab->zName, NULL);
>>
>> @@ -3417,7 +3428,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, pIndex->pTable->zName, pIndex->zName);
>>
>> Please, fix them.
>>
>> > On 4 Apr 2018, at 19:34, Hollow111 <hollow653@gmail.com> wrote:
>> >
>> > @@ -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
>> > + )
>>
>> This bracket should be on previous line.
>>
>> > {
>> > - if(idx_name) {
>> > + if(idx_name != NULL) {
>> > sqlite3NestedParse(parse,
>> > "DELETE FROM \"_sql_stat1\" WHERE (\"idx\"=%Q AND "
>> > "\"tbl\"=%Q)",
>> >
>>
>>
> After considering the remarks changes were made for the newer build of
> Tarantool 2.0:
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..7ca4191 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2128,19 +2128,33 @@ sqliteViewResetAll(sqlite3 * db)
> /**
> * Remove entries from the _sql_stat1 and _sql_stat4
> * system spaces after a DROP INDEX or DROP TABLE command.
> - *
> - * @param pParse Parsing context.
> - * @param zType Type of entry to be deleted:
> - * 'idx' or 'tbl' string literal.
> - * @param zName Name of index or table.
> + *
> + * @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_spaces(Parse * pParse, const char *zType, const char
> *zName)
> +sql_clear_stat_tables(Parse *parse, const char *table_name,
> + const char *idx_name)
> {
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
> - zType, zName);
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
> - zType, zName);
> + if (idx_name != NULL) {
> + 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);
> + }
> }
>
> /**
> @@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct
> SrcList *table_name_list,
> * tuple with corresponding space_id from _space.
> */
>
> - sql_clear_stat_spaces(parse_context, "tbl", space_name);
> + sql_clear_stat_tables(parse_context, space_name, NULL);
> struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name);
> sqlite3FkDropTable(parse_context, table_name_list, tab);
> sql_code_drop_table(parse_context, space, is_view);
> @@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct
> SrcList *index_name_list,
> * But firstly, delete statistics since schema
> * changes after DDL.
> */
> - sql_clear_stat_spaces(parse_context, "idx", index->def->name);
> + sql_clear_stat_tables(parse_context, table_name, index->def->name);
> int record_reg = ++parse_context->nMem;
> int space_id_reg = ++parse_context->nMem;
> sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>
>
[-- Attachment #2: Type: text/html, Size: 8033 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-07 2:12 ` Hollow111
@ 2018-04-09 12:16 ` n.pettik
2018-04-12 6:06 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-09 12:16 UTC (permalink / raw)
To: Hollow111; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]
Hello. I have noticed that you are using 4 spaces as an indentation.
According to our codestyle, we use tab symbol, instead of spaces,
which is equal to 8 spaces.
Also, I don’t see any point in renaming function:
’sql_clear_stat_spaces’ -> ’sql_clear_stat_tables’.
I would even say that ‘spaces’ is more appropriate name,
since stat tables in fact are Tarantool’s system spaces.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..7ca4191 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2128,19 +2128,33 @@ sqliteViewResetAll(sqlite3 * db)
> /**
> * Remove entries from the _sql_stat1 and _sql_stat4
> * system spaces after a DROP INDEX or DROP TABLE command.
> - *
> - * @param pParse Parsing context.
> - * @param zType Type of entry to be deleted:
> - * 'idx' or 'tbl' string literal.
> - * @param zName Name of index or table.
> + *
> + * @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_spaces(Parse * pParse, const char *zType, const char *zName)
> +sql_clear_stat_tables(Parse *parse, const char *table_name,
> + const char *idx_name)
> {
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
> - zType, zName);
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
> - zType, zName);
> + if (idx_name != NULL) {
> + 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);
> + }
> }
>
>
[-- Attachment #2: Type: text/html, Size: 4882 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-09 12:16 ` n.pettik
@ 2018-04-12 6:06 ` Hollow111
2018-04-13 8:50 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-12 6:06 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 5064 bytes --]
Hello. Here's the diff for the changes:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 92f3cb6..0287183 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2129,18 +2129,32 @@ sqliteViewResetAll(sqlite3 * db)
* Remove entries from the _sql_stat1 and _sql_stat4
* system spaces after a DROP INDEX or DROP TABLE command.
*
- * @param pParse Parsing context.
- * @param zType Type of entry to be deleted:
- * 'idx' or 'tbl' string literal.
- * @param zName Name of index or table.
+ * @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_spaces(Parse * pParse, const char *zType, const char *zName)
+sql_clear_stat_spaces(Parse *parse, const char *table_name,
+ const char *idx_name)
{
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
- zType, zName);
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
- zType, zName);
+ if (idx_name != NULL) {
+ 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);
+ }
}
/**
@@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct
SrcList *table_name_list,
* tuple with corresponding space_id from _space.
*/
- sql_clear_stat_spaces(parse_context, "tbl", space_name);
+ sql_clear_stat_spaces(parse_context, space_name, NULL);
struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name);
sqlite3FkDropTable(parse_context, table_name_list, tab);
sql_code_drop_table(parse_context, space, is_view);
@@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct
SrcList *index_name_list,
* But firstly, delete statistics since schema
* changes after DDL.
*/
- sql_clear_stat_spaces(parse_context, "idx", index->def->name);
+ sql_clear_stat_spaces(parse_context, table_name, index->def->name);
int record_reg = ++parse_context->nMem;
int space_id_reg = ++parse_context->nMem;
sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
пн, 9 апр. 2018 г. в 15:16, n.pettik <korablev@tarantool.org>:
> Hello. I have noticed that you are using 4 spaces as an indentation.
> According to our codestyle, we use tab symbol, instead of spaces,
> which is equal to 8 spaces.
>
> Also, I don’t see any point in renaming function:
> ’sql_clear_stat_spaces’ -> ’sql_clear_stat_tables’.
> I would even say that ‘spaces’ is more appropriate name,
> since stat tables in fact are Tarantool’s system spaces.
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 92f3cb6..7ca4191 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -2128,19 +2128,33 @@ sqliteViewResetAll(sqlite3 * db)
>> /**
>> * Remove entries from the _sql_stat1 and _sql_stat4
>> * system spaces after a DROP INDEX or DROP TABLE command.
>> - *
>> - * @param pParse Parsing context.
>> - * @param zType Type of entry to be deleted:
>> - * 'idx' or 'tbl' string literal.
>> - * @param zName Name of index or table.
>> + *
>> + * @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_spaces(Parse * pParse, const char *zType, const char
>> *zName)
>> +sql_clear_stat_tables(Parse *parse, const char *table_name,
>> + const char *idx_name)
>> {
>> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE
>> \"%s\"=%Q",
>> - zType, zName);
>> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE
>> \"%s\"=%Q",
>> - zType, zName);
>> + if (idx_name != NULL) {
>> + 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);
>> + }
>> }
>>
>>
>>
>
>
[-- Attachment #2: Type: text/html, Size: 8920 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-12 6:06 ` Hollow111
@ 2018-04-13 8:50 ` n.pettik
2018-04-14 4:29 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-13 8:50 UTC (permalink / raw)
To: N. Tatunov; +Cc: tarantool-patches
Hello. I still notice several codestyle violations.
This patch can’t be pushed until all of them are fixed.
Please, consider them.
> On 12 Apr 2018, at 09:06, Hollow111 <hollow653@gmail.com> wrote:
>
> Hello. Here's the diff for the changes:
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..0287183 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2129,18 +2129,32 @@ sqliteViewResetAll(sqlite3 * db)
> * Remove entries from the _sql_stat1 and _sql_stat4
> * system spaces after a DROP INDEX or DROP TABLE command.
> *
> - * @param pParse Parsing context.
> - * @param zType Type of entry to be deleted:
> - * 'idx' or 'tbl' string literal.
> - * @param zName Name of index or table.
> + * @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_spaces(Parse * pParse, const char *zType, const char *zName)
> +sql_clear_stat_spaces(Parse *parse, const char *table_name,
> + const char *idx_name)
Arguments which you carry to the next line, should start right below previous:
void f(x int, ….
y char, ….
z double …);
> {
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
> - zType, zName);
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
> - zType, zName);
> + if (idx_name != NULL) {
> + sqlite3NestedParse(parse,
Wrong indentation.
> + "DELETE FROM \"_sql_stat1\" WHERE (\"idx\"=%Q AND "
> + "\"tbl\"=%Q)",
> + idx_name, table_name);
> + sqlite3NestedParse(parse,
> + "DELETE FROM \"_sql_stat4\" WHERE (\"idx\"=%Q AND “
Arguments which you carry to the next line, should start right below previous.
> + "\"tbl\"=%Q)",
> + idx_name, table_name);
> + } else {
> + sqlite3NestedParse(parse,
> + "DELETE FROM \"_sql_stat1\" WHERE \"tbl\"=%Q”,
Arguments which you carry to the next line, should start right below previous.
> + table_name);
> + sqlite3NestedParse(parse,
> + "DELETE FROM \"_sql_stat4\" WHERE \"tbl\"=%Q",
> + table_name);
> + }
Wrong indentation.
> }
>
> /**
> @@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list,
> * tuple with corresponding space_id from _space.
> */
>
> - sql_clear_stat_spaces(parse_context, "tbl", space_name);
> + sql_clear_stat_spaces(parse_context, space_name, NULL);
> struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name);
> sqlite3FkDropTable(parse_context, table_name_list, tab);
> sql_code_drop_table(parse_context, space, is_view);
> @@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list,
> * But firstly, delete statistics since schema
> * changes after DDL.
> */
> - sql_clear_stat_spaces(parse_context, "idx", index->def->name);
> + sql_clear_stat_spaces(parse_context, table_name, index->def->name);
> int record_reg = ++parse_context->nMem;
> int space_id_reg = ++parse_context->nMem;
> sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-13 8:50 ` n.pettik
@ 2018-04-14 4:29 ` Hollow111
2018-04-14 8:13 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-14 4:29 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]
Hello. I'd like to ask about 'one kind of corrections".
пт, 13 апр. 2018 г. в 12:50, n.pettik <korablev@tarantool.org>:
> Hello. I still notice several codestyle violations.
> This patch can’t be pushed until all of them are fixed.
> Please, consider them.
>
> > On 12 Apr 2018, at 09:06, Hollow111 <hollow653@gmail.com> wrote:
> >
> > Hello. Here's the diff for the changes:
> >
> > diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> > index 92f3cb6..0287183 100644
> > --- a/src/box/sql/build.c
> > +++ b/src/box/sql/build.c
> > @@ -2129,18 +2129,32 @@ sqliteViewResetAll(sqlite3 * db)
> > * Remove entries from the _sql_stat1 and _sql_stat4
> > * system spaces after a DROP INDEX or DROP TABLE command.
> > *
> > - * @param pParse Parsing context.
> > - * @param zType Type of entry to be deleted:
> > - * 'idx' or 'tbl' string literal.
> > - * @param zName Name of index or table.
> > + * @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_spaces(Parse * pParse, const char *zType, const char
> *zName)
> > +sql_clear_stat_spaces(Parse *parse, const char *table_name,
> > + const char *idx_name)
>
> Arguments which you carry to the next line, should start right below
> previous:
>
> void f(x int, ….
> y char, ….
> z double …);
>
I couldnt find this in "C style guide" (
https://tarantool.io/en/doc/2.0/dev_guide/c_style_guide.html)
Moreover according to this guide:
"Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation...".
Which means we're supposed to violate this rule
in case first argument is positioned not on the same range
from left corner of screen as any amount of 8-character tabs.
Maybe I'm wrong at smth but I'd like to have an explanation.
> > {
> > - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE
> \"%s\"=%Q",
> > - zType, zName);
> > - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE
> \"%s\"=%Q",
> > - zType, zName);
> > + if (idx_name != NULL) {
> > + sqlite3NestedParse(parse,
>
> Wrong indentation.
>
> > + "DELETE FROM \"_sql_stat1\" WHERE
> (\"idx\"=%Q AND "
> > + "\"tbl\"=%Q)",
> > + idx_name, table_name);
> > + sqlite3NestedParse(parse,
> > + "DELETE FROM \"_sql_stat4\" WHERE
> (\"idx\"=%Q AND “
>
> Arguments which you carry to the next line, should start right below
> previous.
>
> > + "\"tbl\"=%Q)",
> > + idx_name, table_name);
> > + } else {
> > + sqlite3NestedParse(parse,
> > + "DELETE FROM \"_sql_stat1\" WHERE
> \"tbl\"=%Q”,
>
> Arguments which you carry to the next line, should start right below
> previous.
>
> > + table_name);
> > + sqlite3NestedParse(parse,
> > + "DELETE FROM \"_sql_stat4\" WHERE \"tbl\"=%Q",
> > + table_name);
> > + }
>
> Wrong indentation.
>
> > }
> >
> > /**
> > @@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct
> SrcList *table_name_list,
> > * tuple with corresponding space_id from _space.
> > */
> >
> > - sql_clear_stat_spaces(parse_context, "tbl", space_name);
> > + sql_clear_stat_spaces(parse_context, space_name, NULL);
> > struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash,
> space_name);
> > sqlite3FkDropTable(parse_context, table_name_list, tab);
> > sql_code_drop_table(parse_context, space, is_view);
> > @@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct
> SrcList *index_name_list,
> > * But firstly, delete statistics since schema
> > * changes after DDL.
> > */
> > - sql_clear_stat_spaces(parse_context, "idx", index->def->name);
> > + sql_clear_stat_spaces(parse_context, table_name, index->def->name);
> > int record_reg = ++parse_context->nMem;
> > int space_id_reg = ++parse_context->nMem;
> > sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>
>
[-- Attachment #2: Type: text/html, Size: 6814 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-14 4:29 ` Hollow111
@ 2018-04-14 8:13 ` n.pettik
2018-04-15 6:09 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-14 8:13 UTC (permalink / raw)
To: Hollow111; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]
> On 14 Apr 2018, at 07:29, Hollow111 <hollow653@gmail.com> wrote:
>
> Hello. I'd like to ask about 'one kind of corrections”.
>
> Arguments which you carry to the next line, should start right below previous:
>
> void f(x int, ….
> y char, ….
> z double …);
>
> I couldnt find this in "C style guide" (https://tarantool.io/en/doc/2.0/dev_guide/c_style_guide.html <https://tarantool.io/en/doc/2.0/dev_guide/c_style_guide.html>)
> Moreover according to this guide:
> "Outside of comments, documentation and except in Kconfig, spaces are never used for indentation...".
> Which means we're supposed to violate this rule
In fact, they are used. Probably, we should update and clarify this point in docs.
You use tabs as much as possible. To precisely align argument lists, you have
to use spaces:
return_value very_long_function_name_bla_bla_bla(int first_argument,
int second_argument)
To make ’second_argument’ be under first, you should use 8 tabs + 1 space.
Overall, you can inspect other source files from box/ to understand how
source files should be formatted. Also, I advise you to make spaces and tabs visible.
For instance, see how it was done in box/space.c 106 : space_create()
> in case first argument is positioned not on the same range
> from left corner of screen as any amount of 8-character tabs.
> Maybe I'm wrong at smth but I'd like to have an explanation.
[-- Attachment #2: Type: text/html, Size: 4236 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-14 8:13 ` n.pettik
@ 2018-04-15 6:09 ` Hollow111
2018-04-15 6:35 ` Hollow111
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-15 6:09 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 4117 bytes --]
Hello. Changes have been made.
Diff:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 92f3cb6..ce5878c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2129,18 +2129,32 @@ sqliteViewResetAll(sqlite3 * db)
* Remove entries from the _sql_stat1 and _sql_stat4
* system spaces after a DROP INDEX or DROP TABLE command.
*
- * @param pParse Parsing context.
- * @param zType Type of entry to be deleted:
- * 'idx' or 'tbl' string literal.
- * @param zName Name of index or table.
+ * @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_spaces(Parse * pParse, const char *zType, const char *zName)
+sql_clear_stat_spaces(Parse *parse, const char *table_name,
+ const char *idx_name)
{
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
- zType, zName);
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
- zType, zName);
+ if (idx_name != NULL) {
+ 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);
+ }
}
/**
@@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct
SrcList *table_name_list,
* tuple with corresponding space_id from _space.
*/
- sql_clear_stat_spaces(parse_context, "tbl", space_name);
+ sql_clear_stat_spaces(parse_context, space_name, NULL);
struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name);
sqlite3FkDropTable(parse_context, table_name_list, tab);
sql_code_drop_table(parse_context, space, is_view);
@@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct
SrcList *index_name_list,
* But firstly, delete statistics since schema
* changes after DDL.
*/
- sql_clear_stat_spaces(parse_context, "idx", index->def->name);
+ sql_clear_stat_spaces(parse_context, table_name, index->def->name);
int record_reg = ++parse_context->nMem;
int space_id_reg = ++parse_context->nMem;
sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
сб, 14 апр. 2018 г. в 11:13, n.pettik <korablev@tarantool.org>:
>
> On 14 Apr 2018, at 07:29, Hollow111 <hollow653@gmail.com> wrote:
>
> Hello. I'd like to ask about 'one kind of corrections”.
>
>
>
>> Arguments which you carry to the next line, should start right below
>> previous:
>>
>> void f(x int, ….
>> y char, ….
>> z double …);
>>
>
> I couldnt find this in "C style guide" (
> https://tarantool.io/en/doc/2.0/dev_guide/c_style_guide.html)
> Moreover according to this guide:
> "Outside of comments, documentation and except in Kconfig, spaces are
> never used for indentation...".
> Which means we're supposed to violate this rule
>
>
> In fact, they are used. Probably, we should update and clarify this point
> in docs.
> You use tabs as much as possible. To precisely align argument lists, you
> have
> to use spaces:
>
> return_value very_long_function_name_bla_bla_bla(int first_argument,
> int second_argument)
>
> To make ’second_argument’ be under first, you should use 8 tabs + 1 space.
> Overall, you can inspect other source files from box/ to understand how
> source files should be formatted. Also, I advise you to make spaces and
> tabs visible.
> For instance, see how it was done in box/space.c 106 : space_create()
>
> in case first argument is positioned not on the same range
> from left corner of screen as any amount of 8-character tabs.
> Maybe I'm wrong at smth but I'd like to have an explanation.
>
>
>
[-- Attachment #2: Type: text/html, Size: 8945 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-15 6:09 ` Hollow111
@ 2018-04-15 6:35 ` Hollow111
2018-04-15 22:41 ` n.pettik
0 siblings, 1 reply; 16+ messages in thread
From: Hollow111 @ 2018-04-15 6:35 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 6073 bytes --]
I'm sorry have just noticed few mistakes.
Changed it:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 92f3cb6..c6185e4 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2129,18 +2129,36 @@ sqliteViewResetAll(sqlite3 * db)
* Remove entries from the _sql_stat1 and _sql_stat4
* system spaces after a DROP INDEX or DROP TABLE command.
*
- * @param pParse Parsing context.
- * @param zType Type of entry to be deleted:
- * 'idx' or 'tbl' string literal.
- * @param zName Name of index or table.
+ * @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_spaces(Parse * pParse, const char *zType, const char *zName)
+sql_clear_stat_spaces(Parse *parse, const char *table_name,
+ const char *idx_name)
{
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
- zType, zName);
- sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
- zType, zName);
+ if (idx_name != NULL) {
+ 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);
+ }
}
вс, 15 апр. 2018 г. в 9:09, Hollow111 <hollow653@gmail.com>:
> Hello. Changes have been made.
> Diff:
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..ce5878c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2129,18 +2129,32 @@ sqliteViewResetAll(sqlite3 * db)
> * Remove entries from the _sql_stat1 and _sql_stat4
> * system spaces after a DROP INDEX or DROP TABLE command.
> *
> - * @param pParse Parsing context.
> - * @param zType Type of entry to be deleted:
> - * 'idx' or 'tbl' string literal.
> - * @param zName Name of index or table.
> + * @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_spaces(Parse * pParse, const char *zType, const char
> *zName)
> +sql_clear_stat_spaces(Parse *parse, const char *table_name,
> + const char *idx_name)
> {
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat1\" WHERE \"%s\"=%Q",
> - zType, zName);
> - sqlite3NestedParse(pParse, "DELETE FROM \"_sql_stat4\" WHERE \"%s\"=%Q",
> - zType, zName);
> + if (idx_name != NULL) {
> + 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);
> + }
> }
>
> /**
> @@ -2325,7 +2339,7 @@ sql_drop_table(struct Parse *parse_context, struct
> SrcList *table_name_list,
> * tuple with corresponding space_id from _space.
> */
>
> - sql_clear_stat_spaces(parse_context, "tbl", space_name);
> + sql_clear_stat_spaces(parse_context, space_name, NULL);
> struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name);
> sqlite3FkDropTable(parse_context, table_name_list, tab);
> sql_code_drop_table(parse_context, space, is_view);
> @@ -3328,7 +3342,7 @@ sql_drop_index(struct Parse *parse_context, struct
> SrcList *index_name_list,
> * But firstly, delete statistics since schema
> * changes after DDL.
> */
> - sql_clear_stat_spaces(parse_context, "idx", index->def->name);
> + sql_clear_stat_spaces(parse_context, table_name, index->def->name);
> int record_reg = ++parse_context->nMem;
> int space_id_reg = ++parse_context->nMem;
> sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg);
>
>
> сб, 14 апр. 2018 г. в 11:13, n.pettik <korablev@tarantool.org>:
>
>>
>> On 14 Apr 2018, at 07:29, Hollow111 <hollow653@gmail.com> wrote:
>>
>> Hello. I'd like to ask about 'one kind of corrections”.
>>
>>
>>
>>> Arguments which you carry to the next line, should start right below
>>> previous:
>>>
>>> void f(x int, ….
>>> y char, ….
>>> z double …);
>>>
>>
>> I couldnt find this in "C style guide" (
>> https://tarantool.io/en/doc/2.0/dev_guide/c_style_guide.html)
>> Moreover according to this guide:
>> "Outside of comments, documentation and except in Kconfig, spaces are
>> never used for indentation...".
>> Which means we're supposed to violate this rule
>>
>>
>> In fact, they are used. Probably, we should update and clarify this point
>> in docs.
>> You use tabs as much as possible. To precisely align argument lists, you
>> have
>> to use spaces:
>>
>> return_value very_long_function_name_bla_bla_bla(int first_argument,
>> int second_argument)
>>
>> To make ’second_argument’ be under first, you should use 8 tabs + 1 space.
>> Overall, you can inspect other source files from box/ to understand how
>> source files should be formatted. Also, I advise you to make spaces and
>> tabs visible.
>> For instance, see how it was done in box/space.c 106 : space_create()
>>
>> in case first argument is positioned not on the same range
>> from left corner of screen as any amount of 8-character tabs.
>> Maybe I'm wrong at smth but I'd like to have an explanation.
>>
>>
>>
[-- Attachment #2: Type: text/html, Size: 13110 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-15 6:35 ` Hollow111
@ 2018-04-15 22:41 ` n.pettik
2018-04-16 13:19 ` Kirill Yukhin
0 siblings, 1 reply; 16+ messages in thread
From: n.pettik @ 2018-04-15 22:41 UTC (permalink / raw)
To: Hollow111; +Cc: tarantool-patches, Kirill Yukhin
> On 15 Apr 2018, at 09:35, Hollow111 <hollow653@gmail.com> wrote:
>
> I'm sorry have just noticed few mistakes.
> Changed it:
Thanks. Now this patch looks OK to me.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
2018-04-15 22:41 ` n.pettik
@ 2018-04-16 13:19 ` Kirill Yukhin
0 siblings, 0 replies; 16+ messages in thread
From: Kirill Yukhin @ 2018-04-16 13:19 UTC (permalink / raw)
To: n.pettik; +Cc: Hollow111, tarantool-patches
Hello,
On 16 апр 01:41, n.pettik wrote:
>
> > On 15 Apr 2018, at 09:35, Hollow111 <hollow653@gmail.com> wrote:
> >
> > I'm sorry have just noticed few mistakes.
> > Changed it:
>
> Thanks. Now this patch looks OK to me.
I've checked in the changes to 2.x branch
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-04-16 13:19 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-03 21:37 [tarantool-patches] [PATCH 2/2] sql: statistics removal after dropping an index N.Tatunov
2018-04-04 14:06 ` [tarantool-patches] " n.pettik
2018-04-04 15:46 ` Hollow111
2018-04-04 16:11 ` n.pettik
2018-04-04 16:34 ` Hollow111
2018-04-05 18:01 ` n.pettik
[not found] ` <CAEi+_aq5oyeB0cbnxAXXjQqu=h+PCGaaZuLkk3p33yq371+Xog@mail.gmail.com>
2018-04-07 2:12 ` Hollow111
2018-04-09 12:16 ` n.pettik
2018-04-12 6:06 ` Hollow111
2018-04-13 8:50 ` n.pettik
2018-04-14 4:29 ` Hollow111
2018-04-14 8:13 ` n.pettik
2018-04-15 6:09 ` Hollow111
2018-04-15 6:35 ` Hollow111
2018-04-15 22:41 ` n.pettik
2018-04-16 13:19 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox