From: "n.pettik" <korablev@tarantool.org>
To: "N. Tatunov" <hollow653@gmail.com>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index
Date: Wed, 4 Apr 2018 17:06:00 +0300 [thread overview]
Message-ID: <EF6BDB21-E3F3-4633-8B29-EA9C38441B4B@tarantool.org> (raw)
In-Reply-To: <1522791436-8221-1-git-send-email-hollow653@gmail.com>
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
>
>
next prev parent reply other threads:[~2018-04-04 14:06 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-03 21:37 [tarantool-patches] " N.Tatunov
2018-04-04 14:06 ` n.pettik [this message]
2018-04-04 15:46 ` [tarantool-patches] " 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=EF6BDB21-E3F3-4633-8B29-EA9C38441B4B@tarantool.org \
--to=korablev@tarantool.org \
--cc=hollow653@gmail.com \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox