From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8D4432B783 for ; Wed, 4 Apr 2018 10:06:04 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id N3Qtp7tUXtf6 for ; Wed, 4 Apr 2018 10:06:04 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BF0C02B72E for ; Wed, 4 Apr 2018 10:06:03 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: statistics removal after dropping an index From: "n.pettik" In-Reply-To: <1522791436-8221-1-git-send-email-hollow653@gmail.com> Date: Wed, 4 Apr 2018 17:06:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1522791436-8221-1-git-send-email-hollow653@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "N. Tatunov" Cc: tarantool-patches@freelists.org Please, don=E2=80=99t hurry when working on patch. Consider carefully each fix and comment: there is no any deadline. You don=E2=80=99t 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=E2=80=99t change tests (since = they are OK), so don=E2=80=99t include them in provided diff. > On 4 Apr 2018, at 00:37, N.Tatunov wrote: >=20 > 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. >=20 > Closes: #3264 > --- >=20 > Branch: = https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3264-stat-table-e= ntries-removal > Issue: https://github.com/tarantool/tarantool/issues/3264 >=20 > 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 >=20 > 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 */ >=20 > /* > - * 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 =E2=80=98doxygen comments=E2=80= =99. > 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=E2=80=99t need =E2=80=98p=E2=80=99 and =E2=80=98z=E2=80=99 = 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 =3D 1; i <=3D 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\"=3D%Q", > - zTab, zType, zName); > - } > - } > + int i, j; You don=E2=80=99t 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 =3D 0; =E2=80=A6) - is OK. > + if(strcmp(zType, "idx") =3D=3D 0) As you suggest, you can get rid of this =E2=80=99type=E2=80=99. Instead, = just check index name on nullability. > + for(i =3D 1, j =3D 1; i <=3D 2; i++, j++) { This cycle-for is completely unreadable. Don=E2=80=99t make things to be = complicated: it is better to use *less* beautiful, but more obvious and clear = approach. Don=E2=80=99t 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(=E2=80=A6, =E2=80=9D =E2=80=A6 WHERE tbl =3D = stat1=E2=80=9D); sqlite3NestedParse(=E2=80=A6, =E2=80=9D =E2=80=A6 WHERE tbl =3D = stat4=E2=80=9D);=20 > + char zTab[24]; > + sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * = j); > + sqlite3NestedParse(pParse, > + "DELETE FROM \"%s\" WHERE (\"idx\"=3D%Q AND " > + "\"tbl\"=3D%Q)", > + zTab, idx_name, table_name); > + } > + else > + for(i =3D 1, j =3D 1; i <=3D 2; i++, j++) { > + char zTab[24]; > + sqlite3_snprintf(sizeof(zTab), zTab, "_sql_stat%d", i * = j); > + sqlite3NestedParse(pParse, > + "DELETE FROM \"%s\" WHERE \"tbl\"=3D%Q", > + zTab, table_name); > + } > } >=20 > /* > @@ -2415,7 +2424,7 @@ sqlite3DropTable(Parse * pParse, SrcList * = pName, int isView, int noErr) > */ >=20 > 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); >=20 > @@ -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 =3D ++pParse->nMem; > int space_id_reg =3D ++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 =3D 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=3D] > + - ['T1', 'T1', '1', '0', '0', !!binary kQE=3D] > + - ['T2', 'I1', '1', '0', '0', !!binary kQI=3D] > + - ['T2', 'T2', '1', '0', '0', !!binary kQE=3D] > +... > +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=3D] > + - ['T2', 'I1', '1', '0', '0', !!binary kQI=3D] > + - ['T2', 'T2', '1', '0', '0', !!binary kQE=3D] > +... > +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=3D] > + - ['T1', 'T1', '1', '0', '0', !!binary kQE=3D] > + - ['T2', 'I1', '1', '0', '0', !!binary kQI=3D] > + - ['T2', 'T2', '1', '0', '0', !!binary kQE=3D] > +... > +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=3D] > + - ['T1', 'T1', '1', '0', '0', !!binary kQE=3D] > + - ['T2', 'T2', '1', '0', '0', !!binary kQE=3D] > +... > +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 =3D 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;") > --=20 > 2.7.4 >=20 >=20