From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: imeevma@tarantool.org Subject: [PATCH v2 3/3] box: remove _sql_stat1 and _sql_stat4 system tables Date: Wed, 3 Apr 2019 19:58:38 +0300 Message-Id: <8907dd9a025b0cafe9858289e4e3e2d64618f12b.1554310018.git.imeevma@gmail.com> MIME-Version: 1.0 In-Reply-To: References: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: vdavydov.dev@gmail.com Cc: tarantool-patches@freelists.org List-ID: Hi! Thank you for review. My answers and new version below. On 4/3/19 11:35 AM, Vladimir Davydov wrote: > On Tue, Apr 02, 2019 at 12:25:49PM +0300, imeevma@tarantool.org wrote: >> These tables won't be used anymore and should be deleted. > > Please mention that this breaks backward compatibility. > Added to commit-message. >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 4fac020..7beeee1 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -87,7 +87,7 @@ sql_load_schema() >> * statistics (_sql_stat1 and _sql_stat4). Thus, we can >> * skip statistics loading. >> */ >> - struct space *stat = space_by_id(BOX_SQL_STAT1_ID); >> + struct space *stat = space_by_name("_sql_stat1"); > > I don't understand this change: even though you removed the tables you > still expect them to be accessible by name here and in a few other > places. At any rate, the comment above needs to be refreshed. > I did this to free BOX_SQL_STAT1_ID and BOX_SQL_STAT4_ID so I could remove them. These functions are currently unused. Moved this fix to a different commit. >> @@ -2478,19 +2460,17 @@ sql_drop_index(struct Parse *parse_context, struct SrcList *index_name_list, >> } >> goto exit_drop_index; >> } >> - struct index *index = space_index(space, index_id); >> - assert(index != NULL); >> >> /* >> * Generate code to remove entry from _index space >> * But firstly, delete statistics since schema >> * changes after DDL. >> */ >> - sql_clear_stat_spaces(parse_context, table_name, index->def->name); >> int record_reg = ++parse_context->nMem; >> int space_id_reg = ++parse_context->nMem; >> + int index_id_reg =++parse_context->nMem; > > Nit: a space is missing. > Fixed. > Anyway, this change looks like it doesn't have anything to do with stat > tables removal. If so, please factor it out into a separate patch with a > proper justification. > Actually it does. When I removed the cleanup of the statistics tables from the functions mentioned, the SQL tests started to fail due to a segmentation fault. I moved this fix to a different commit. New version: commit 8907dd9a025b0cafe9858289e4e3e2d64618f12b Author: Mergen Imeev Date: Wed Apr 3 19:05:09 2019 +0300 box: remove _sql_stat1 and _sql_stat4 system tables These tables won't be used anymore and should be deleted. This patch breaks backward compatibility. Part of #2843 diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap index 1dba789..871a93f 100644 Binary files a/src/box/bootstrap.snap and b/src/box/bootstrap.snap differ diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index 9dfc97b..5a5875e 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -556,10 +556,6 @@ box_lua_space_init(struct lua_State *L) lua_setfield(L, -2, "CLUSTER_ID"); lua_pushnumber(L, BOX_TRIGGER_ID); lua_setfield(L, -2, "TRIGGER_ID"); - lua_pushnumber(L, BOX_SQL_STAT1_ID); - lua_setfield(L, -2, "SQL_STAT1_ID"); - lua_pushnumber(L, BOX_SQL_STAT4_ID); - lua_setfield(L, -2, "SQL_STAT4_ID"); lua_pushnumber(L, BOX_FK_CONSTRAINT_ID); lua_setfield(L, -2, "FK_CONSTRAINT_ID"); lua_pushnumber(L, BOX_TRUNCATE_ID); diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua index 275aaf7..89d6e3d 100644 --- a/src/box/lua/upgrade.lua +++ b/src/box/lua/upgrade.lua @@ -553,33 +553,6 @@ local function upgrade_to_2_1_0() _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false }, {{1, 'unsigned'}}} - local stat1_ft = {{name='tbl', type='string'}, - {name='idx', type='string'}, - {name='stat', type='string'}} - local stat4_ft = {{name='tbl', type='string'}, - {name='idx', type='string'}, - {name='neq', type='string'}, - {name='nlt', type='string'}, - {name='ndlt', type='string'}, - {name='sample', type='scalar'}} - - log.info("create space _sql_stat1") - _space:insert{box.schema.SQL_STAT1_ID, ADMIN, '_sql_stat1', 'memtx', 0, - MAP, stat1_ft} - - log.info("create index primary on _sql_stat1") - _index:insert{box.schema.SQL_STAT1_ID, 0, 'primary', 'tree', - {unique = true}, {{0, 'string'}, {1, 'string'}}} - - log.info("create space _sql_stat4") - _space:insert{box.schema.SQL_STAT4_ID, ADMIN, '_sql_stat4', 'memtx', 0, - MAP, stat4_ft} - - log.info("create index primary on _sql_stat4") - _index:insert{box.schema.SQL_STAT4_ID, 0, 'primary', 'tree', - {unique = true}, {{0, 'string'}, {1, 'string'}, - {5, 'scalar'}}} - local fk_constr_ft = {{name='name', type='string'}, {name='child_id', type='unsigned'}, {name='parent_id', type='unsigned'}, diff --git a/src/box/schema.cc b/src/box/schema.cc index 79d0de4..9a55c2f 100644 --- a/src/box/schema.cc +++ b/src/box/schema.cc @@ -452,22 +452,6 @@ schema_init() sc_space_new(BOX_INDEX_ID, "_index", key_parts, 2, &alter_space_on_replace_index, &on_stmt_begin_index); - /* _sql_stat1 - a simpler statistics on space, seen in SQL. */ - key_parts[0].fieldno = 0; /* space name */ - key_parts[0].type = FIELD_TYPE_STRING; - key_parts[1].fieldno = 1; /* index name */ - key_parts[1].type = FIELD_TYPE_STRING; - sc_space_new(BOX_SQL_STAT1_ID, "_sql_stat1", key_parts, 2, NULL, NULL); - - /* _sql_stat4 - extensive statistics on space, seen in SQL. */ - key_parts[0].fieldno = 0; /* space name */ - key_parts[0].type = FIELD_TYPE_STRING; - key_parts[1].fieldno = 1; /* index name */ - key_parts[1].type = FIELD_TYPE_STRING; - key_parts[2].fieldno = 5; /* sample */ - key_parts[2].type = FIELD_TYPE_SCALAR; - sc_space_new(BOX_SQL_STAT4_ID, "_sql_stat4", key_parts, 3, NULL, NULL); - /* _fk_сonstraint - foreign keys constraints. */ key_parts[0].fieldno = 0; /* constraint name */ key_parts[0].type = FIELD_TYPE_STRING; diff --git a/src/box/schema_def.h b/src/box/schema_def.h index a760ecc..eeeeb95 100644 --- a/src/box/schema_def.h +++ b/src/box/schema_def.h @@ -106,9 +106,6 @@ enum { BOX_TRUNCATE_ID = 330, /** Space id of _space_sequence. */ BOX_SPACE_SEQUENCE_ID = 340, - /** Space ids for SQL statictics. */ - BOX_SQL_STAT1_ID = 348, - BOX_SQL_STAT4_ID = 349, /** Space id of _fk_constraint. */ BOX_FK_CONSTRAINT_ID = 356, /** End of the reserved range of system spaces. */ diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua index a914db5..62a78d6 100755 --- a/test/app-tap/tarantoolctl.test.lua +++ b/test/app-tap/tarantoolctl.test.lua @@ -403,8 +403,8 @@ do check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1", "\n", 3) check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1 --replica 2", "\n", 3) check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 2", "\n", 0) - check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 23) - check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 49) + check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 21) + check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 47) end) end) diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result index fa6099d..379f6c5 100644 --- a/test/box-py/bootstrap.result +++ b/test/box-py/bootstrap.result @@ -73,11 +73,6 @@ box.space._space:select{} 'type': 'unsigned'}]] - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'sequence_id', 'type': 'unsigned'}, {'name': 'is_generated', 'type': 'boolean'}]] - - [348, 1, '_sql_stat1', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'stat', 'type': 'string'}]] - - [349, 1, '_sql_stat4', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'neq', 'type': 'string'}, {'name': 'nlt', 'type': 'string'}, - {'name': 'ndlt', 'type': 'string'}, {'name': 'sample', 'type': 'scalar'}]] - [356, 1, '_fk_constraint', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'child_id', 'type': 'unsigned'}, {'name': 'parent_id', 'type': 'unsigned'}, {'name': 'is_deferred', 'type': 'boolean'}, {'name': 'match', 'type': 'string'}, @@ -133,9 +128,6 @@ box.space._index:select{} - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]] - [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]] - [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]] - - [348, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string']]] - - [349, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string'], [ - 5, 'scalar']]] - [356, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'unsigned']]] - [356, 1, 'child_id', 'tree', {'unique': false}, [[1, 'unsigned']]] ... diff --git a/test/box/access_misc.result b/test/box/access_misc.result index 4ffeb38..36ebfae 100644 --- a/test/box/access_misc.result +++ b/test/box/access_misc.result @@ -813,11 +813,6 @@ box.space._space:select() 'type': 'unsigned'}]] - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'sequence_id', 'type': 'unsigned'}, {'name': 'is_generated', 'type': 'boolean'}]] - - [348, 1, '_sql_stat1', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'stat', 'type': 'string'}]] - - [349, 1, '_sql_stat4', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'neq', 'type': 'string'}, {'name': 'nlt', 'type': 'string'}, - {'name': 'ndlt', 'type': 'string'}, {'name': 'sample', 'type': 'scalar'}]] - [356, 1, '_fk_constraint', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'child_id', 'type': 'unsigned'}, {'name': 'parent_id', 'type': 'unsigned'}, {'name': 'is_deferred', 'type': 'boolean'}, {'name': 'match', 'type': 'string'}, diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result index fd8b142..ae04266 100644 --- a/test/box/access_sysview.result +++ b/test/box/access_sysview.result @@ -230,11 +230,11 @@ box.session.su('guest') ... #box.space._vspace:select{} --- -- 24 +- 22 ... #box.space._vindex:select{} --- -- 50 +- 48 ... #box.space._vuser:select{} --- @@ -262,7 +262,7 @@ box.session.su('guest') ... #box.space._vindex:select{} --- -- 50 +- 48 ... #box.space._vuser:select{} --- diff --git a/test/box/alter.result b/test/box/alter.result index 37bc51c..c1b1de1 100644 --- a/test/box/alter.result +++ b/test/box/alter.result @@ -228,9 +228,6 @@ _index:select{} - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]] - [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]] - [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]] - - [348, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string']]] - - [349, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string'], [ - 5, 'scalar']]] - [356, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'unsigned']]] - [356, 1, 'child_id', 'tree', {'unique': false}, [[1, 'unsigned']]] ... diff --git a/test/sql/delete.result b/test/sql/delete.result index 30f6b67..59178ba 100644 --- a/test/sql/delete.result +++ b/test/sql/delete.result @@ -79,9 +79,9 @@ box.execute("DROP TABLE t2;") -- gh-2201: TRUNCATE TABLE operation. -- -- can't truncate system table. -box.execute("TRUNCATE TABLE \"_sql_stat1\";") +box.execute("TRUNCATE TABLE \"_fk_constraint\";") --- -- error: Can't truncate a system space, space '_sql_stat1' +- error: Can't truncate a system space, space '_fk_constraint' ... box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT, b TEXT);") --- diff --git a/test/sql/delete.test.lua b/test/sql/delete.test.lua index 96a18b7..61685e7 100644 --- a/test/sql/delete.test.lua +++ b/test/sql/delete.test.lua @@ -44,7 +44,7 @@ box.execute("DROP TABLE t2;") -- -- can't truncate system table. -box.execute("TRUNCATE TABLE \"_sql_stat1\";") +box.execute("TRUNCATE TABLE \"_fk_constraint\";") box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT, b TEXT);") box.execute("INSERT INTO t1 VALUES(1, 1, 'one');") diff --git a/test/sql/triggers.result b/test/sql/triggers.result index bb8d80c..7dc44ea 100644 --- a/test/sql/triggers.result +++ b/test/sql/triggers.result @@ -267,10 +267,10 @@ box.space._trigger:insert(tuple) --- - error: 'SQL error: cannot create AFTER trigger on view: V1' ... -space_id = box.space._sql_stat1.id +space_id = box.space._fk_constraint.id --- ... -tuple = {"T1T", space_id, {sql = [[create trigger t1t instead of update on "_sql_stat1" for each row begin delete from t1 WHERE a=old.a+2; end;]]}} +tuple = {"T1T", space_id, {sql = [[create trigger t1t instead of update on "_fk_constraint" for each row begin delete from t1 WHERE a=old.a+2; end;]]}} --- ... box.space._trigger:insert(tuple) diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua index e50699b..7749402 100644 --- a/test/sql/triggers.test.lua +++ b/test/sql/triggers.test.lua @@ -89,8 +89,8 @@ box.space._trigger:insert(tuple) tuple = {"V1T", space_id, {sql = [[create trigger v1t AFTER update on v1 for each row begin delete from t1 WHERE a=old.a+2; end;]]}} box.space._trigger:insert(tuple) -space_id = box.space._sql_stat1.id -tuple = {"T1T", space_id, {sql = [[create trigger t1t instead of update on "_sql_stat1" for each row begin delete from t1 WHERE a=old.a+2; end;]]}} +space_id = box.space._fk_constraint.id +tuple = {"T1T", space_id, {sql = [[create trigger t1t instead of update on "_fk_constraint" for each row begin delete from t1 WHERE a=old.a+2; end;]]}} box.space._trigger:insert(tuple) box.execute("DROP VIEW V1;") diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result index cec8813..9569674 100644 --- a/test/sql/upgrade.result +++ b/test/sql/upgrade.result @@ -29,30 +29,10 @@ box.space._space.index['name']:get('_trigger') - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id', 'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]] ... -box.space._space.index['name']:get('_sql_stat1') ---- -- [348, 1, '_sql_stat1', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'stat', 'type': 'string'}]] -... -box.space._space.index['name']:get('_sql_stat4') ---- -- [349, 1, '_sql_stat4', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx', - 'type': 'string'}, {'name': 'neq', 'type': 'string'}, {'name': 'nlt', 'type': 'string'}, - {'name': 'ndlt', 'type': 'string'}, {'name': 'sample', 'type': 'scalar'}]] -... box.space._index:get({box.space._space.index['name']:get('_trigger').id, 0}) --- - [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]] ... -box.space._index:get({box.space._space.index['name']:get('_sql_stat1').id, 0}) ---- -- [348, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string']]] -... -box.space._index:get({box.space._space.index['name']:get('_sql_stat4').id, 0}) ---- -- [349, 0, 'primary', 'tree', {'unique': true}, [[0, 'string'], [1, 'string'], [5, - 'scalar']]] -... box.space._schema:format() --- - [{'type': 'string', 'name': 'key'}, {'type': 'any', 'name': 'value', 'is_nullable': true}] diff --git a/test/sql/upgrade.test.lua b/test/sql/upgrade.test.lua index 351e79d..e6cdae1 100644 --- a/test/sql/upgrade.test.lua +++ b/test/sql/upgrade.test.lua @@ -10,12 +10,8 @@ test_run:switch('upgrade') -- test system tables box.space._space.index['name']:get('_trigger') -box.space._space.index['name']:get('_sql_stat1') -box.space._space.index['name']:get('_sql_stat4') box.space._index:get({box.space._space.index['name']:get('_trigger').id, 0}) -box.space._index:get({box.space._space.index['name']:get('_sql_stat1').id, 0}) -box.space._index:get({box.space._space.index['name']:get('_sql_stat4').id, 0}) box.space._schema:format() diff --git a/test/wal_off/alter.result b/test/wal_off/alter.result index b4c6a92..becdf13 100644 --- a/test/wal_off/alter.result +++ b/test/wal_off/alter.result @@ -28,7 +28,7 @@ end; ... #spaces; --- -- 65509 +- 65511 ... -- cleanup for k, v in pairs(spaces) do -- 2.7.4