From d5ba5631c0b4e90ee044b37162c5f0a0e0ea2e2d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 01:04:17 +0800 Subject: [PATCH 01/11] RUNTIME_ASSERT for renaming table running into rename directories Remove useless rename on read test Signed-off-by: JaySon-Huang --- dbms/src/Databases/test/gtest_database.cpp | 36 +++++----- .../Storages/DeltaMerge/DeltaMergeStore.cpp | 18 ++--- .../tests/gtest_dm_delta_merge_store.cpp | 3 + .../tests/gtest_dm_storage_delta_merge.cpp | 9 +-- dbms/src/Storages/StorageDeltaMerge.cpp | 67 +++++-------------- .../raft/schema/rename_on_read.test | 38 ----------- .../raft/txn_mock/partition_table.test | 13 +--- 7 files changed, 48 insertions(+), 136 deletions(-) delete mode 100644 tests/delta-merge-test/raft/schema/rename_on_read.test diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index 6b8bbc17348..b469a571570 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -273,18 +273,15 @@ try EXPECT_EQ(managed_storage->getDatabaseName(), db_name); } - const String to_tbl_name = "t_112"; + const String to_tbl_display_name = "tbl_test"; { // Rename table - typeid_cast(db.get())->renameTable(ctx, tbl_name, *db, to_tbl_name, db_name, to_tbl_name); + typeid_cast(db.get())->renameTable(ctx, tbl_name, *db, tbl_name, db_name, to_tbl_display_name); - auto old_storage = db->tryGetTable(ctx, tbl_name); - ASSERT_EQ(old_storage, nullptr); - - auto storage = db->tryGetTable(ctx, to_tbl_name); + auto storage = db->tryGetTable(ctx, tbl_name); ASSERT_NE(storage, nullptr); EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); - EXPECT_EQ(storage->getTableName(), to_tbl_name); + EXPECT_EQ(storage->getTableName(), tbl_name); auto managed_storage = std::dynamic_pointer_cast(storage); EXPECT_EQ(managed_storage->getDatabaseName(), db_name); @@ -294,13 +291,13 @@ try // Drop table auto drop_query = std::make_shared(); drop_query->database = db_name; - drop_query->table = to_tbl_name; + drop_query->table = tbl_name; drop_query->if_exists = false; ASTPtr ast_drop_query = drop_query; InterpreterDropQuery drop_interpreter(ast_drop_query, ctx); drop_interpreter.execute(); - auto storage = db->tryGetTable(ctx, to_tbl_name); + auto storage = db->tryGetTable(ctx, tbl_name); ASSERT_EQ(storage, nullptr); } @@ -391,18 +388,18 @@ try EXPECT_EQ(managed_storage->getDatabaseName(), db_name); } - const String to_tbl_name = "t_112"; + const String to_tbl_display_name = "tbl_test"; { // Rename table - typeid_cast(db.get())->renameTable(ctx, tbl_name, *db2, to_tbl_name, db2_name, to_tbl_name); + typeid_cast(db.get())->renameTable(ctx, tbl_name, *db2, tbl_name, db2_name, to_tbl_display_name); auto old_storage = db->tryGetTable(ctx, tbl_name); ASSERT_EQ(old_storage, nullptr); - auto storage = db2->tryGetTable(ctx, to_tbl_name); + auto storage = db2->tryGetTable(ctx, tbl_name); ASSERT_NE(storage, nullptr); EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); - EXPECT_EQ(storage->getTableName(), to_tbl_name); + EXPECT_EQ(storage->getTableName(), tbl_name); auto managed_storage = std::dynamic_pointer_cast(storage); EXPECT_EQ(managed_storage->getDatabaseName(), db2_name); @@ -412,13 +409,13 @@ try // Drop table auto drop_query = std::make_shared(); drop_query->database = db2_name; - drop_query->table = to_tbl_name; + drop_query->table = tbl_name; drop_query->if_exists = false; ASTPtr ast_drop_query = drop_query; InterpreterDropQuery drop_interpreter(ast_drop_query, ctx); drop_interpreter.execute(); - auto storage = db2->tryGetTable(ctx, to_tbl_name); + auto storage = db2->tryGetTable(ctx, tbl_name); ASSERT_EQ(storage, nullptr); } @@ -501,18 +498,17 @@ try EXPECT_FALSE(db->empty(ctx)); EXPECT_TRUE(db->isTableExist(ctx, tbl_name)); - const String to_tbl_name = "t_112"; // Rename table to another database, and mock crash by failed point FailPointHelper::enableFailPoint(FailPoints::exception_before_rename_table_old_meta_removed); ASSERT_THROW( - typeid_cast(db.get())->renameTable(ctx, tbl_name, *db2, to_tbl_name, db2_name, to_tbl_name), + typeid_cast(db.get())->renameTable(ctx, tbl_name, *db2, tbl_name, db2_name, tbl_name), DB::Exception); { // After fail point triggled we should have both meta file in disk Poco::File old_meta_file{db->getTableMetadataPath(tbl_name)}; ASSERT_TRUE(old_meta_file.exists()); - Poco::File new_meta_file(db2->getTableMetadataPath(to_tbl_name)); + Poco::File new_meta_file(db2->getTableMetadataPath(tbl_name)); ASSERT_TRUE(new_meta_file.exists()); // Old table should remain in db auto old_storage = db->tryGetTable(ctx, tbl_name); @@ -527,10 +523,10 @@ try ThreadPool thread_pool(2); db2->loadTables(ctx, &thread_pool, true); - Poco::File new_meta_file(db2->getTableMetadataPath(to_tbl_name)); + Poco::File new_meta_file(db2->getTableMetadataPath(tbl_name)); ASSERT_FALSE(new_meta_file.exists()); - auto storage = db2->tryGetTable(ctx, to_tbl_name); + auto storage = db2->tryGetTable(ctx, tbl_name); ASSERT_EQ(storage, nullptr); } diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 900239e623a..21271f6934a 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -357,18 +357,12 @@ void DeltaMergeStore::setUpBackgroundTask(const DMContextPtr & dm_context) void DeltaMergeStore::rename(String /*new_path*/, bool clean_rename, String new_database_name, String new_table_name) { - if (clean_rename) - { - path_pool.rename(new_database_name, new_table_name, clean_rename); - } - else - { - LOG_FMT_WARNING(log, "Applying heavy renaming for table {}.{} to {}.{}", db_name, table_name, new_database_name, new_table_name); - - // Remove all background task first - shutdown(); - path_pool.rename(new_database_name, new_table_name, clean_rename); // rename for multi-disk - } + RUNTIME_ASSERT(clean_rename, + log, + "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", + new_database_name, + new_table_name); + path_pool->rename(new_database_name, new_table_name, clean_rename); // TODO: replacing these two variables is not atomic, but could be good enough? table_name.swap(new_table_name); diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp index 0259d228038..c21e39bd16a 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -24,6 +25,8 @@ #include #include #include +#include +#include #include #include diff --git a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp index ac03b509f18..4406cf06289 100644 --- a/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp +++ b/dbms/src/Storages/DeltaMerge/tests/gtest_dm_storage_delta_merge.cpp @@ -207,10 +207,12 @@ try // Rename database name before store object is created. const String new_db_name = "new_" + storage->getDatabaseName(); - storage->rename(path_name, new_db_name, table_name, table_name); + const String new_display_table_name = "new_" + storage->getTableName(); + storage->rename(path_name, new_db_name, table_name, new_display_table_name); ASSERT_FALSE(storage->storeInited()); ASSERT_EQ(storage->getTableName(), table_name); ASSERT_EQ(storage->getDatabaseName(), new_db_name); + ASSERT_EQ(storage->getTableInfo().name, new_display_table_name); // prepare block data Block sample; @@ -231,9 +233,8 @@ try } // TiFlash always use t_{table_id} as table name - String new_table_name = storage->getTableName(); - storage->rename(path_name, new_db_name, new_table_name, new_table_name); - ASSERT_EQ(storage->getTableName(), new_table_name); + storage->rename(path_name, new_db_name, table_name, table_name); + ASSERT_EQ(storage->getTableName(), table_name); ASSERT_EQ(storage->getDatabaseName(), new_db_name); storage->drop(); diff --git a/dbms/src/Storages/StorageDeltaMerge.cpp b/dbms/src/Storages/StorageDeltaMerge.cpp index 6eb6a16736b..31d9ded2551 100644 --- a/dbms/src/Storages/StorageDeltaMerge.cpp +++ b/dbms/src/Storages/StorageDeltaMerge.cpp @@ -1198,59 +1198,26 @@ void StorageDeltaMerge::rename( // For DatabaseTiFlash, simply update store's database is OK. // `store->getTableName() == new_table_name` only keep for mock test. bool clean_rename = !data_path_contains_database_name && getTableName() == new_table_name; - if (likely(clean_rename)) + RUNTIME_ASSERT(clean_rename, + log, + "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", + new_database_name, + new_table_name); + if (storeInited()) { - if (storeInited()) - { - _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); - return; - } - std::lock_guard lock(store_mutex); - if (storeInited()) - { - _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); - } - else - { - table_column_info->db_name = new_database_name; - table_column_info->table_name = new_table_name; - } + _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); return; } - - /// Note that this routine is only left for CI tests. `clean_rename` should always be true in production env. - auto & store = getAndMaybeInitStore(); - - // For DatabaseOrdinary, we need to rename data path, then recreate a new store. - const String new_path = new_path_to_db + "/" + new_table_name; - - if (Poco::File{new_path}.exists()) - throw Exception( - fmt::format("Target path already exists: {}", new_path), - ErrorCodes::DIRECTORY_ALREADY_EXISTS); - - // flush store and then reset store to new path - store->flushCache(global_context, RowKeyRange::newAll(is_common_handle, rowkey_column_size)); - ColumnDefines table_column_defines = store->getTableColumns(); - ColumnDefine handle_column_define = store->getHandle(); - DeltaMergeStore::Settings settings = store->getSettings(); - - // remove background tasks - store->shutdown(); - // rename directories for multi disks - store->rename(new_path, clean_rename, new_database_name, new_table_name); - // generate a new store - store = std::make_shared( - global_context, - data_path_contains_database_name, - new_database_name, - new_table_name, - tidb_table_info.id, - std::move(table_column_defines), - std::move(handle_column_define), - is_common_handle, - rowkey_column_size, - settings); + std::lock_guard lock(store_mutex); + if (storeInited()) + { + _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); + } + else + { + table_column_info->db_name = new_database_name; + table_column_info->table_name = new_table_name; + } } String StorageDeltaMerge::getTableName() const diff --git a/tests/delta-merge-test/raft/schema/rename_on_read.test b/tests/delta-merge-test/raft/schema/rename_on_read.test deleted file mode 100644 index 40eb66277a9..00000000000 --- a/tests/delta-merge-test/raft/schema/rename_on_read.test +++ /dev/null @@ -1,38 +0,0 @@ -# Copyright 2022 PingCAP, Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -=> DBGInvoke __enable_schema_sync_service('false') - -=> DBGInvoke __drop_tidb_table(default, test) -=> drop table if exists default.test - -=> DBGInvoke __drop_tidb_table(default, test1) -=> drop table if exists default.test1 - -=> DBGInvoke __set_flush_threshold(1000000, 1000000) - -=> DBGInvoke __mock_tidb_table(default, test, 'col_1 String', '', 'dt') -=> DBGInvoke __refresh_schemas() -=> select * from default.test -=> DBGInvoke __rename_tidb_table(default, test, test1) -=> select * from default.test -=> select * from default.test " --schema_version "1000000 -Received exception from server (version {#WORD}): -Code: 60. DB::Exception: Received from {#WORD} DB::Exception: Table default.test doesn't exist.. -=> select * from default.test1 -=> select * from default.test1 " --schema_version "1000000 - -=> DBGInvoke __drop_tidb_table(default, test1) -=> drop table if exists default.test1 -=> DBGInvoke __enable_schema_sync_service('true') diff --git a/tests/delta-merge-test/raft/txn_mock/partition_table.test b/tests/delta-merge-test/raft/txn_mock/partition_table.test index 2f8e67a61a8..43ace7094e7 100644 --- a/tests/delta-merge-test/raft/txn_mock/partition_table.test +++ b/tests/delta-merge-test/raft/txn_mock/partition_table.test @@ -21,8 +21,6 @@ => drop table if exists default.test_9997 => drop table if exists default.test_9998 => drop table if exists default.test_9999 -=> drop table if exists default.test1_9997 -=> drop table if exists default.test1_9999 => DBGInvoke __mock_tidb_table(default, test, 'col_1 String, col_2 Int64') => DBGInvoke __mock_tidb_partition(default, test, 9999) @@ -94,16 +92,9 @@ │ 0 │ └──────────────┘ -=> DBGInvoke __rename_tidb_table(default, test, test1) -=> DBGInvoke __refresh_schemas() -=> select count(*) from default.test1_9997 -┌─count()─┐ -│ 2 │ -└─────────┘ - => DBGInvoke __drop_tidb_table(default, test1) => DBGInvoke __refresh_schemas() -=> DBGInvoke is_tombstone(default, test1_9999) +=> DBGInvoke is_tombstone(default, test_9999) ┌─is_tombstone(default, test_9999)─┐ │ true │ └──────────────────────────────────┘ @@ -113,8 +104,6 @@ => drop table if exists default.test_9997 => drop table if exists default.test_9998 => drop table if exists default.test_9999 -=> drop table if exists default.test1_9997 -=> drop table if exists default.test1_9999 => DBGInvoke __enable_schema_sync_service('true') => DBGInvoke __clean_up_region() From 65b4f535fbe7adab26b1286a67f2c23be631f239 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 14:20:51 +0800 Subject: [PATCH 02/11] Clean mock tests for rename table Signed-off-by: JaySon-Huang --- tests/delta-merge-test/ddl/alter.test | 16 ------- .../raft/schema/partition_table_restart.test | 40 ++++------------ .../raft/schema/rename_on_write.test | 46 ------------------- .../raft/schema/rename_tables.test | 45 ------------------ .../raft/txn_mock/partition_table.test | 4 +- 5 files changed, 11 insertions(+), 140 deletions(-) delete mode 100644 tests/delta-merge-test/raft/schema/rename_on_write.test delete mode 100644 tests/delta-merge-test/raft/schema/rename_tables.test diff --git a/tests/delta-merge-test/ddl/alter.test b/tests/delta-merge-test/ddl/alter.test index 4bf405ac9e1..c058e5cdc81 100644 --- a/tests/delta-merge-test/ddl/alter.test +++ b/tests/delta-merge-test/ddl/alter.test @@ -72,21 +72,5 @@ └───┴──────┴───────┴──────┘ -## rename table ->> drop table if exists dm_test_renamed ->> rename table dm_test to dm_test_renamed ->> select * from dm_test -Received exception from server (version {#WORD}): -Code: 60. DB::Exception: Received from {#WORD} DB::Exception: Table default.dm_test doesn't exist.. - ->> select * from dm_test_renamed -┌─a─┬────b─┬─────c─┬────d─┐ -│ 1 │ 0 │ 0 │ \N │ -│ 2 │ 1024 │ 65535 │ 4096 │ -│ 3 │ 2048 │ 65536 │ \N │ -└───┴──────┴───────┴──────┘ - - ## Clean up >> drop table if exists dm_test ->> drop table if exists dm_test_renamed diff --git a/tests/delta-merge-test/raft/schema/partition_table_restart.test b/tests/delta-merge-test/raft/schema/partition_table_restart.test index 893bb617af4..c7a5e488111 100644 --- a/tests/delta-merge-test/raft/schema/partition_table_restart.test +++ b/tests/delta-merge-test/raft/schema/partition_table_restart.test @@ -15,16 +15,11 @@ => DBGInvoke __enable_schema_sync_service('false') => DBGInvoke __drop_tidb_table(default, test) -=> DBGInvoke __drop_tidb_table(default, test1) => DBGInvoke __refresh_schemas() => drop table if exists default.test => drop table if exists default.test_9999 => drop table if exists default.test_9998 => drop table if exists default.test_9997 -=> drop table if exists default.test1 -=> drop table if exists default.test1_9999 -=> drop table if exists default.test1_9998 -=> drop table if exists default.test1_9997 => DBGInvoke __mock_tidb_table(default, test, 'col_1 String, col_2 Int64', '', 'dt') => DBGInvoke __mock_tidb_partition(default, test, 9999) @@ -38,38 +33,23 @@ => DBGInvoke __reset_schemas() => DBGInvoke __add_column_to_tidb_table(default, test, 'col_3 Nullable(Int8)') -=> DBGInvoke __rename_tidb_table(default, test, test1) => DBGInvoke __refresh_schemas() - -=> show tables -┌─name───────┐ -│ test1 │ -│ test1_9997 │ -│ test1_9998 │ -│ test1_9999 │ -└────────────┘ -=> select col_2 from default.test1_9997 -=> select * from default.test_9997 -Received exception from server (version {#WORD}): -Code: 60. DB::Exception: Received from {#WORD} DB::Exception: Table default.test_9997 doesn't exist.. -=> select * from default.test_9998 -Received exception from server (version {#WORD}): -Code: 60. DB::Exception: Received from {#WORD} DB::Exception: Table default.test_9998 doesn't exist.. +=> select col_2 from default.test_9997 => DBGInvoke __reset_schemas() -=> DBGInvoke __drop_tidb_partition(default, test1, 9997) +=> DBGInvoke __drop_tidb_partition(default, test, 9997) => DBGInvoke __refresh_schemas() -=> DBGInvoke is_tombstone(default, test1_9997) -┌─is_tombstone(default, test1_9997)─┐ +=> DBGInvoke is_tombstone(default, test_9997) +┌─is_tombstone(default, test_9997)─┐ │ true │ └───────────────────────────────────┘ -=> select * from default.test1_9999 +=> select * from default.test_9999 -=> DBGInvoke __drop_tidb_table(default, test1) +=> DBGInvoke __drop_tidb_table(default, test) => DBGInvoke __refresh_schemas() -=> drop table if exists default.test1 -=> drop table if exists default.test1_9999 -=> drop table if exists default.test1_9998 -=> drop table if exists default.test1_9997 +=> drop table if exists default.test +=> drop table if exists default.test_9999 +=> drop table if exists default.test_9998 +=> drop table if exists default.test_9997 => DBGInvoke __enable_schema_sync_service('true') diff --git a/tests/delta-merge-test/raft/schema/rename_on_write.test b/tests/delta-merge-test/raft/schema/rename_on_write.test deleted file mode 100644 index 6ad2c809ce6..00000000000 --- a/tests/delta-merge-test/raft/schema/rename_on_write.test +++ /dev/null @@ -1,46 +0,0 @@ -# Copyright 2022 PingCAP, Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -#TODO: We can not mock this situation, ignore for now -#RETURN - -=> DBGInvoke __enable_schema_sync_service('false') - -=> DBGInvoke __drop_tidb_table(default, test) -=> drop table if exists default.test - -=> DBGInvoke __drop_tidb_table(default, test1) -=> drop table if exists default.test1 - -=> DBGInvoke __set_flush_threshold(1000000, 1000000) - -=> DBGInvoke __mock_tidb_table(default, test, 'col_1 String', '', 'dt') -=> DBGInvoke __refresh_schemas() -=> DBGInvoke __put_region(4, 0, 100, default, test) -=> select col_1 from default.test -=> DBGInvoke __add_column_to_tidb_table(default, test, 'col_2 Nullable(Int8)') -=> DBGInvoke __rename_tidb_table(default, test, test1) -#For DeltaTree, each write will trigger schema sync. -=> DBGInvoke __raft_insert_row(default, test1, 4, 50, 'test1', 1) -=> select * from default.test -Received exception from server (version {#WORD}): -Code: 60. DB::Exception: Received from {#WORD} DB::Exception: Table default.test doesn't exist.. -=> select * from default.test1 -┌─col_1─┬─_tidb_rowid─┬─col_2─┐ -│ test1 │ 50 │ 1 │ -└───────┴─────────────┴───────┘ - -=> DBGInvoke __drop_tidb_table(default, test1) -=> drop table if exists default.test1 -=> DBGInvoke __enable_schema_sync_service('true') diff --git a/tests/delta-merge-test/raft/schema/rename_tables.test b/tests/delta-merge-test/raft/schema/rename_tables.test deleted file mode 100644 index 7c65d46d3e3..00000000000 --- a/tests/delta-merge-test/raft/schema/rename_tables.test +++ /dev/null @@ -1,45 +0,0 @@ -# Copyright 2022 PingCAP, Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -# Preparation. -=> DBGInvoke __enable_schema_sync_service('false') - -=> DBGInvoke __drop_tidb_table(default, t1) -=> DBGInvoke __drop_tidb_table(default, t2) -=> drop table if exists default.t1 -=> drop table if exists default.t2 -=> DBGInvoke __refresh_schemas() - -=> DBGInvoke __set_flush_threshold(1000000, 1000000) - - -=> DBGInvoke __create_tidb_tables(default, t1, t2) -# rename table -=> DBGInvoke __rename_tidb_tables(default, t1, r1, default, t2, r2) -=> DBGInvoke __refresh_schemas() -=> select database,name,engine from system.tables where database='default' and name='r1' -┌─database─┬─name─┬─engine─────┐ -│ default │ r1 │ DeltaMerge │ -└──────────┴──────┴────────────┘ -=> select database,name,engine from system.tables where database='default' and name='r2' -┌─database─┬─name─┬─engine─────┐ -│ default │ r2 │ DeltaMerge │ -└──────────┴──────┴────────────┘ - -# clean -=> DBGInvoke __drop_tidb_table(default, r1) -=> DBGInvoke __drop_tidb_table(default, r2) -=> drop table if exists default.r1 -=> drop table if exists default.r2 -=> DBGInvoke __enable_schema_sync_service('true') \ No newline at end of file diff --git a/tests/delta-merge-test/raft/txn_mock/partition_table.test b/tests/delta-merge-test/raft/txn_mock/partition_table.test index 43ace7094e7..84b8044260f 100644 --- a/tests/delta-merge-test/raft/txn_mock/partition_table.test +++ b/tests/delta-merge-test/raft/txn_mock/partition_table.test @@ -17,7 +17,6 @@ => DBGInvoke __drop_tidb_table(default, test) => drop table if exists default.test -=> drop table if exists default.test1 => drop table if exists default.test_9997 => drop table if exists default.test_9998 => drop table if exists default.test_9999 @@ -92,7 +91,7 @@ │ 0 │ └──────────────┘ -=> DBGInvoke __drop_tidb_table(default, test1) +=> DBGInvoke __drop_tidb_table(default, test) => DBGInvoke __refresh_schemas() => DBGInvoke is_tombstone(default, test_9999) ┌─is_tombstone(default, test_9999)─┐ @@ -100,7 +99,6 @@ └──────────────────────────────────┘ => drop table if exists default.test -=> drop table if exists default.test1 => drop table if exists default.test_9997 => drop table if exists default.test_9998 => drop table if exists default.test_9999 From ae8645801696f8f28e17001298334453acddd0f7 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 14:48:39 +0800 Subject: [PATCH 03/11] Clean PathPool::rename Signed-off-by: JaySon-Huang --- .../Storages/DeltaMerge/DeltaMergeStore.cpp | 2 +- dbms/src/Storages/PathPool.cpp | 52 +++---------------- dbms/src/Storages/PathPool.h | 2 +- 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 21271f6934a..ddee8fbfa78 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -362,7 +362,7 @@ void DeltaMergeStore::rename(String /*new_path*/, bool clean_rename, String new_ "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", new_database_name, new_table_name); - path_pool->rename(new_database_name, new_table_name, clean_rename); + path_pool->rename(new_database_name, new_table_name); // TODO: replacing these two variables is not atomic, but could be good enough? table_name.swap(new_table_name); diff --git a/dbms/src/Storages/PathPool.cpp b/dbms/src/Storages/PathPool.cpp index 2e7edd7435b..edb1ede0747 100644 --- a/dbms/src/Storages/PathPool.cpp +++ b/dbms/src/Storages/PathPool.cpp @@ -228,53 +228,15 @@ void StoragePathPool::clearPSV2ObsoleteData() drop_instance_data("data"); } -void StoragePathPool::rename(const String & new_database, const String & new_table, bool clean_rename) +void StoragePathPool::rename(const String & new_database, const String & new_table) { - if (unlikely(new_database.empty() || new_table.empty())) - throw Exception(fmt::format("Can not rename for PathPool to {}.{}", new_database, new_table)); + RUNTIME_CHECK(!new_database.empty() && !new_table.empty(), new_database, new_table); + RUNTIME_CHECK(!path_need_database_name); - if (likely(clean_rename)) - { - // caller ensure that no path need to be renamed. - if (unlikely(path_need_database_name)) - throw Exception("Can not do clean rename with path_need_database_name is true!"); - - std::lock_guard lock{mutex}; - database = new_database; - table = new_table; - } - else - { - if (unlikely(file_provider->isEncryptionEnabled())) - throw Exception("Encryption is only supported when using clean_rename"); - - // Note: changing these path is not atomic, we may lost data if process is crash here. - std::lock_guard lock{mutex}; - // Get root path without database and table - for (auto & info : main_path_infos) - { - Poco::Path p(info.path); - p = p.parent().parent(); - if (path_need_database_name) - p = p.parent(); - auto new_path = getStorePath(p.toString() + "/data", new_database, new_table); - renamePath(info.path, new_path); - info.path = new_path; - } - for (auto & info : latest_path_infos) - { - Poco::Path p(info.path); - p = p.parent().parent(); - if (path_need_database_name) - p = p.parent(); - auto new_path = getStorePath(p.toString() + "/data", new_database, new_table); - renamePath(info.path, new_path); - info.path = new_path; - } - - database = new_database; - table = new_table; - } + // The directories for storing table data is not changed, just rename related names. + std::lock_guard lock{mutex}; + database = new_database; + table = new_table; } void StoragePathPool::drop(bool recursive, bool must_success) diff --git a/dbms/src/Storages/PathPool.h b/dbms/src/Storages/PathPool.h index 4f16511feff..59d8f463e4b 100644 --- a/dbms/src/Storages/PathPool.h +++ b/dbms/src/Storages/PathPool.h @@ -412,7 +412,7 @@ class StoragePathPool void clearPSV2ObsoleteData(); - void rename(const String & new_database, const String & new_table, bool clean_rename); + void rename(const String & new_database, const String & new_table); void drop(bool recursive, bool must_success = true); From 730426750bc24634253bbf057ce38033d3db0023 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 14:54:08 +0800 Subject: [PATCH 04/11] Add more output Signed-off-by: JaySon-Huang --- dbms/src/Storages/PathPool.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/PathPool.cpp b/dbms/src/Storages/PathPool.cpp index edb1ede0747..4c0f696cb3d 100644 --- a/dbms/src/Storages/PathPool.cpp +++ b/dbms/src/Storages/PathPool.cpp @@ -230,8 +230,8 @@ void StoragePathPool::clearPSV2ObsoleteData() void StoragePathPool::rename(const String & new_database, const String & new_table) { - RUNTIME_CHECK(!new_database.empty() && !new_table.empty(), new_database, new_table); - RUNTIME_CHECK(!path_need_database_name); + RUNTIME_CHECK(!new_database.empty() && !new_table.empty(), database, table, new_database, new_table); + RUNTIME_CHECK(!path_need_database_name, database, table, new_database, new_table); // The directories for storing table data is not changed, just rename related names. std::lock_guard lock{mutex}; From f043e9095a07485c8a97b3480009059519b86cf3 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 17:11:20 +0800 Subject: [PATCH 05/11] Fix Signed-off-by: JaySon-Huang --- dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index ddee8fbfa78..8564b05cfe2 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -362,7 +362,7 @@ void DeltaMergeStore::rename(String /*new_path*/, bool clean_rename, String new_ "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", new_database_name, new_table_name); - path_pool->rename(new_database_name, new_table_name); + path_pool.rename(new_database_name, new_table_name); // TODO: replacing these two variables is not atomic, but could be good enough? table_name.swap(new_table_name); From 831115191478c2cda98900eedb88a85aa1365759 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 18:47:05 +0800 Subject: [PATCH 06/11] Move the mock test into unit test Signed-off-by: JaySon-Huang --- dbms/src/Databases/test/gtest_database.cpp | 10 +- dbms/src/Debug/MockSchemaGetter.h | 29 +- dbms/src/Debug/MockTiDB.cpp | 50 ++- dbms/src/Debug/MockTiDB.h | 26 +- dbms/src/Server/RaftConfigParser.h | 5 + dbms/src/Storages/Transaction/PDTiKVClient.h | 5 - dbms/src/Storages/Transaction/TMTContext.cpp | 28 +- dbms/src/Storages/Transaction/TMTContext.h | 4 + dbms/src/TestUtils/TiFlashTestEnv.cpp | 3 +- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 4 + dbms/src/TiDB/Schema/TiDBSchemaSyncer.h | 11 +- .../TiDB/Schema/tests/gtest_schema_sync.cpp | 327 ++++++++++++++++++ .../raft/schema/partition_table_restart.test | 55 --- 13 files changed, 447 insertions(+), 110 deletions(-) create mode 100644 dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp delete mode 100644 tests/delta-merge-test/raft/schema/partition_table_restart.test diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index b469a571570..95aa75520f2 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -97,16 +97,10 @@ class DatabaseTiFlashTest : public ::testing::Test static void recreateMetadataPath() { String path = TiFlashTestEnv::getContext().getPath(); - auto p = path + "/metadata/"; - if (Poco::File file(p); file.exists()) - file.remove(true); - Poco::File{p}.createDirectories(); - + TiFlashTestEnv::tryRemovePath(p, /*recreate=*/true); p = path + "/data/"; - if (Poco::File file(p); file.exists()) - file.remove(true); - Poco::File{p}.createDirectories(); + TiFlashTestEnv::tryRemovePath(p, /*recreate=*/true); } protected: diff --git a/dbms/src/Debug/MockSchemaGetter.h b/dbms/src/Debug/MockSchemaGetter.h index 11c5d97f036..677455e895a 100644 --- a/dbms/src/Debug/MockSchemaGetter.h +++ b/dbms/src/Debug/MockSchemaGetter.h @@ -23,30 +23,33 @@ namespace DB { struct MockSchemaGetter { - TiDB::DBInfoPtr getDatabase(DatabaseID db_id) { return MockTiDB::instance().getDBInfoByID(db_id); } + static TiDB::DBInfoPtr getDatabase(DatabaseID db_id) { return MockTiDB::instance().getDBInfoByID(db_id); } - Int64 getVersion() { return MockTiDB::instance().getVersion(); } + static Int64 getVersion() { return MockTiDB::instance().getVersion(); } - std::optional getSchemaDiff(Int64 version) + static std::optional getSchemaDiff(Int64 version) { return MockTiDB::instance().getSchemaDiff(version); } - bool checkSchemaDiffExists(Int64 version) + static bool checkSchemaDiffExists(Int64 version) { return MockTiDB::instance().checkSchemaDiffExists(version); } - TiDB::TableInfoPtr getTableInfo(DatabaseID, TableID table_id) { return MockTiDB::instance().getTableInfoByID(table_id); } + static TiDB::TableInfoPtr getTableInfo(DatabaseID, TableID table_id) + { + return MockTiDB::instance().getTableInfoByID(table_id); + } - std::vector listDBs() + static std::vector listDBs() { std::vector res; const auto & databases = MockTiDB::instance().getDatabases(); - for (auto it = databases.begin(); it != databases.end(); it++) + for (const auto & database : databases) { - auto db_id = it->second; - auto db_name = it->first; + auto db_id = database.second; + auto db_name = database.first; TiDB::DBInfoPtr db_ptr = std::make_shared(TiDB::DBInfo()); db_ptr->id = db_id; db_ptr->name = db_name; @@ -55,15 +58,15 @@ struct MockSchemaGetter return res; } - std::vector listTables(Int64 db_id) + static std::vector listTables(Int64 db_id) { auto tables_by_id = MockTiDB::instance().getTables(); std::vector res; - for (auto it = tables_by_id.begin(); it != tables_by_id.end(); it++) + for (auto & it : tables_by_id) { - if (it->second->dbID() == db_id) + if (it.second->dbID() == db_id) { - res.push_back(std::make_shared(TiDB::TableInfo(it->second->table_info))); + res.push_back(std::make_shared(TiDB::TableInfo(it.second->table_info))); } } return res; diff --git a/dbms/src/Debug/MockTiDB.cpp b/dbms/src/Debug/MockTiDB.cpp index 99d9625461b..61c359ec298 100644 --- a/dbms/src/Debug/MockTiDB.cpp +++ b/dbms/src/Debug/MockTiDB.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -31,6 +32,8 @@ #include #include +#include + namespace DB { namespace ErrorCodes @@ -350,27 +353,40 @@ Field getDefaultValue(const ASTPtr & default_value_ast) return Field(); } -void MockTiDB::newPartition(const String & database_name, const String & table_name, TableID partition_id, Timestamp tso, bool is_add_part) +TableID MockTiDB::newPartition(TableID belong_logical_table, const String & partition_name, Timestamp tso, bool is_add_part) { std::lock_guard lock(tables_mutex); - TablePtr table = getTableByNameInternal(database_name, table_name); - TableInfo & table_info = table->table_info; + TablePtr logical_table = getTableByID(belong_logical_table); + TableID partition_id = table_id_allocator++; // allocate automatically - const auto & part_def = find_if( - table_info.partition.definitions.begin(), - table_info.partition.definitions.end(), - [&partition_id](PartitionDefinition & part_def) { return part_def.id == partition_id; }); - if (part_def != table_info.partition.definitions.end()) - throw Exception("Mock TiDB table " + database_name + "." + table_name + " already has partition " + std::to_string(partition_id), - ErrorCodes::LOGICAL_ERROR); + return newPartitionImpl(logical_table, partition_id, partition_name, tso, is_add_part); +} + +TableID MockTiDB::newPartition(const String & database_name, const String & table_name, TableID partition_id, Timestamp tso, bool is_add_part) +{ + std::lock_guard lock(tables_mutex); + + TablePtr logical_table = getTableByNameInternal(database_name, table_name); + return newPartitionImpl(logical_table, partition_id, toString(partition_id), tso, is_add_part); +} + +TableID MockTiDB::newPartitionImpl(const TablePtr & logical_table, TableID partition_id, const String & partition_name, Timestamp tso, bool is_add_part) +{ + TableInfo & table_info = logical_table->table_info; + RUNTIME_CHECK_MSG(!logical_table->existPartitionID(partition_id), + "Mock TiDB table {}.{} already has partition {}, table_info={}", + logical_table->database_name, + logical_table->table_name, + partition_id, + table_info.serialize()); table_info.is_partition_table = true; table_info.partition.enable = true; table_info.partition.num++; PartitionDefinition partition_def; partition_def.id = partition_id; - partition_def.name = std::to_string(partition_id); + partition_def.name = partition_name; table_info.partition.definitions.emplace_back(partition_def); table_info.update_timestamp = tso; @@ -380,11 +396,12 @@ void MockTiDB::newPartition(const String & database_name, const String & table_n SchemaDiff diff; diff.type = SchemaActionType::AddTablePartition; - diff.schema_id = table->database_id; - diff.table_id = table->id(); + diff.schema_id = logical_table->database_id; + diff.table_id = logical_table->id(); diff.version = version; version_diff[version] = diff; } + return partition_id; } void MockTiDB::dropPartition(const String & database_name, const String & table_name, TableID partition_id) @@ -631,6 +648,13 @@ TablePtr MockTiDB::getTableByNameInternal(const String & database_name, const St return it->second; } +TablePtr MockTiDB::getTableByID(TableID table_id) +{ + if (auto it = tables_by_id.find(table_id); it != tables_by_id.end()) + return it->second; + throw Exception(fmt::format("Mock TiDB table does not exists, table_id={}", table_id), ErrorCodes::UNKNOWN_TABLE); +} + TiDB::TableInfoPtr MockTiDB::getTableInfoByID(TableID table_id) { auto it = tables_by_id.find(table_id); diff --git a/dbms/src/Debug/MockTiDB.h b/dbms/src/Debug/MockTiDB.h index 261e547b13a..1f0df73031e 100644 --- a/dbms/src/Debug/MockTiDB.h +++ b/dbms/src/Debug/MockTiDB.h @@ -37,23 +37,32 @@ class MockTiDB : public ext::Singleton public: Table(const String & database_name, DatabaseID database_id, const String & table_name, TiDB::TableInfo && table_info); - TableID id() { return table_info.id; } - DatabaseID dbID() { return database_id; } + TableID id() const { return table_info.id; } + DatabaseID dbID() const { return database_id; } ColumnID allocColumnID() { return ++col_id; } - bool isPartitionTable() { return table_info.is_partition_table; } + bool isPartitionTable() const { return table_info.is_partition_table; } - std::vector getPartitionIDs() + std::vector getPartitionIDs() const { std::vector partition_ids; std::for_each( table_info.partition.definitions.begin(), table_info.partition.definitions.end(), - [&](const TiDB::PartitionDefinition & part_def) { partition_ids.emplace_back(part_def.id); }); + [&](const auto & part_def) { partition_ids.emplace_back(part_def.id); }); return partition_ids; } + bool existPartitionID(TableID part_id) const + { + const auto & part_def = find_if( + table_info.partition.definitions.begin(), + table_info.partition.definitions.end(), + [&part_id](const auto & part_def) { return part_def.id == part_id; }); + return part_def != table_info.partition.definitions.end(); + } + TiDB::TableInfo table_info; private: @@ -89,7 +98,8 @@ class MockTiDB : public ext::Singleton DatabaseID newDataBase(const String & database_name); - void newPartition(const String & database_name, const String & table_name, TableID partition_id, Timestamp tso, bool); + TableID newPartition(const String & database_name, const String & table_name, TableID partition_id, Timestamp tso, bool); + TableID newPartition(TableID belong_logical_table, const String & partition_name, Timestamp tso, bool); void dropPartition(const String & database_name, const String & table_name, TableID partition_id); @@ -135,13 +145,15 @@ class MockTiDB : public ext::Singleton std::unordered_map getTables() { return tables_by_id; } - Int64 getVersion() { return version; } + Int64 getVersion() const { return version; } TableID newTableID() { return table_id_allocator++; } private: + TableID newPartitionImpl(const TablePtr & logical_table, TableID partition_id, const String & partition_name, Timestamp tso, bool is_add_part); TablePtr dropTableInternal(Context & context, const String & database_name, const String & table_name, bool drop_regions); TablePtr getTableByNameInternal(const String & database_name, const String & table_name); + TablePtr getTableByID(TableID table_id); private: std::mutex tables_mutex; diff --git a/dbms/src/Server/RaftConfigParser.h b/dbms/src/Server/RaftConfigParser.h index c42304289e2..659a42d76fb 100644 --- a/dbms/src/Server/RaftConfigParser.h +++ b/dbms/src/Server/RaftConfigParser.h @@ -37,8 +37,13 @@ struct TiFlashRaftConfig std::unordered_set ignore_databases{"system"}; // Actually it is "flash.service_addr" std::string flash_server_addr; + + // Use PageStorage V1 for kvstore or not. + // TODO: remove this config bool enable_compatible_mode = true; + bool for_unit_test = false; + static constexpr TiDB::StorageEngine DEFAULT_ENGINE = TiDB::StorageEngine::DT; TiDB::StorageEngine engine = DEFAULT_ENGINE; TiDB::SnapshotApplyMethod snapshot_apply_method = TiDB::SnapshotApplyMethod::DTFile_Directory; diff --git a/dbms/src/Storages/Transaction/PDTiKVClient.h b/dbms/src/Storages/Transaction/PDTiKVClient.h index e5801cc7fae..111ae9862ed 100644 --- a/dbms/src/Storages/Transaction/PDTiKVClient.h +++ b/dbms/src/Storages/Transaction/PDTiKVClient.h @@ -31,11 +31,6 @@ #include -// We define a shared ptr here, because TMTContext / SchemaSyncer / IndexReader all need to -// `share` the resource of cluster. -using KVClusterPtr = std::shared_ptr; - - namespace DB { struct PDClientHelper diff --git a/dbms/src/Storages/Transaction/TMTContext.cpp b/dbms/src/Storages/Transaction/TMTContext.cpp index c67e3b7755e..09f63b087ff 100644 --- a/dbms/src/Storages/Transaction/TMTContext.cpp +++ b/dbms/src/Storages/Transaction/TMTContext.cpp @@ -27,6 +27,8 @@ #include #include +#include + namespace DB { // default batch-read-index timeout is 10_000ms. @@ -38,6 +40,28 @@ const int64_t DEFAULT_WAIT_REGION_READY_TIMEOUT_SEC = 20 * 60; const int64_t DEFAULT_READ_INDEX_WORKER_TICK_MS = 10; +static SchemaSyncerPtr createSchemaSyncer(bool exist_pd_addr, bool for_unit_test, const KVClusterPtr & cluster) +{ + if (exist_pd_addr) + { + // product env + // Get DBInfo/TableInfo from TiKV, and create table with names `t_${table_id}` + return std::static_pointer_cast( + std::make_shared>(cluster)); + } + else if (!for_unit_test) + { + // mock test + // Get DBInfo/TableInfo from MockTiDB, and create table with its display names + return std::static_pointer_cast( + std::make_shared>(cluster)); + } + // unit test. + // Get DBInfo/TableInfo from MockTiDB, but create table with names `t_${table_id}` + return std::static_pointer_cast( + std::make_shared>(cluster)); +} + TMTContext::TMTContext(Context & context_, const TiFlashRaftConfig & raft_config, const pingcap::ClusterConfig & cluster_config) : context(context_) , kvstore(std::make_shared(context, raft_config.snapshot_apply_method)) @@ -47,9 +71,7 @@ TMTContext::TMTContext(Context & context_, const TiFlashRaftConfig & raft_config , cluster(raft_config.pd_addrs.empty() ? std::make_shared() : std::make_shared(raft_config.pd_addrs, cluster_config)) , ignore_databases(raft_config.ignore_databases) - , schema_syncer(raft_config.pd_addrs.empty() - ? std::static_pointer_cast(std::make_shared>(cluster)) - : std::static_pointer_cast(std::make_shared>(cluster))) + , schema_syncer(createSchemaSyncer(!raft_config.pd_addrs.empty(), raft_config.for_unit_test, cluster)) , mpp_task_manager(std::make_shared( std::make_unique( context.getSettingsRef().task_scheduler_thread_soft_limit, diff --git a/dbms/src/Storages/Transaction/TMTContext.h b/dbms/src/Storages/Transaction/TMTContext.h index 41cc3b6557c..62e1d574275 100644 --- a/dbms/src/Storages/Transaction/TMTContext.h +++ b/dbms/src/Storages/Transaction/TMTContext.h @@ -44,6 +44,10 @@ using GCManagerPtr = std::shared_ptr; struct TiFlashRaftConfig; +// We define a shared ptr here, because TMTContext / SchemaSyncer / IndexReader all need to +// `share` the resource of cluster. +using KVClusterPtr = std::shared_ptr; + class TMTContext : private boost::noncopyable { public: diff --git a/dbms/src/TestUtils/TiFlashTestEnv.cpp b/dbms/src/TestUtils/TiFlashTestEnv.cpp index 5ef80f48872..be88200c53d 100644 --- a/dbms/src/TestUtils/TiFlashTestEnv.cpp +++ b/dbms/src/TestUtils/TiFlashTestEnv.cpp @@ -79,8 +79,9 @@ void TiFlashTestEnv::initializeGlobalContext(Strings testdata_path, PageStorageR TiFlashRaftConfig raft_config; - raft_config.ignore_databases = {"default", "system"}; + raft_config.ignore_databases = {"system"}; raft_config.engine = TiDB::StorageEngine::DT; + raft_config.for_unit_test = true; global_context->createTMTContext(raft_config, pingcap::ClusterConfig()); global_context->setDeltaIndexManager(1024 * 1024 * 100 /*100MB*/); diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 85287bebc0f..9db15685c74 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1361,8 +1361,12 @@ void SchemaBuilder::syncAllSchema() LOG_INFO(log, "Loaded all schemas."); } +// product env template struct SchemaBuilder; +// mock test template struct SchemaBuilder; +// unit test +template struct SchemaBuilder; // end namespace } // namespace DB diff --git a/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h b/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h index 880828c1259..baabb507f17 100644 --- a/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h +++ b/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h @@ -27,21 +27,22 @@ namespace DB { +using KVClusterPtr = std::shared_ptr; namespace ErrorCodes { extern const int FAIL_POINT_ERROR; }; -template +template struct TiDBSchemaSyncer : public SchemaSyncer { using Getter = std::conditional_t; - using NameMapper = std::conditional_t; + using NameMapper = std::conditional_t; KVClusterPtr cluster; - const Int64 maxNumberOfDiffs = 100; + static constexpr Int64 maxNumberOfDiffs = 100; Int64 cur_version; @@ -51,8 +52,8 @@ struct TiDBSchemaSyncer : public SchemaSyncer Poco::Logger * log; - TiDBSchemaSyncer(KVClusterPtr cluster_) - : cluster(cluster_) + explicit TiDBSchemaSyncer(KVClusterPtr cluster_) + : cluster(std::move(cluster_)) , cur_version(0) , log(&Poco::Logger::get("SchemaSyncer")) {} diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp new file mode 100644 index 00000000000..a9ee22baa67 --- /dev/null +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -0,0 +1,327 @@ +// Copyright 2022 PingCAP, Ltd. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "Interpreters/InterpreterDropQuery.h" +#include "Parsers/ASTDropQuery.h" +#include "Parsers/IAST.h" + +namespace DB +{ +namespace FailPoints +{ +extern const char exception_before_rename_table_old_meta_removed[]; +extern const char force_context_path[]; +} // namespace FailPoints +namespace tests +{ + +class SchemaSyncTest : public ::testing::Test +{ +public: + SchemaSyncTest() + : global_ctx(TiFlashTestEnv::getGlobalContext()) + {} + + static void SetUpTestCase() + { + try + { + registerStorages(); + } + catch (DB::Exception &) + { + // Maybe another test has already registed, ignore exception here. + } + + FailPointHelper::enableFailPoint(FailPoints::force_context_path); + } + + static void TearDownTestCase() + { + FailPointHelper::disableFailPoint(FailPoints::force_context_path); + } + + void SetUp() override + { + // disable schema sync timer + global_ctx.getSchemaSyncService().reset(); + recreateMetadataPath(); + } + + void TearDown() override + { + for (auto & [db_name, db_id] : MockTiDB::instance().getDatabases()) + { + UNUSED(db_id); + MockTiDB::instance().dropDB(global_ctx, db_name, false); + } + for (auto & db : global_ctx.getDatabases()) + { + mustDropSyncedDatabase(db.first); + } + + // restore schema sync timer + if (!global_ctx.getSchemaSyncService()) + global_ctx.initializeSchemaSyncService(); + } + + void refreshSchema() + { + auto & flash_ctx = global_ctx.getTMTContext(); + auto schema_syncer = flash_ctx.getSchemaSyncer(); + try + { + schema_syncer->syncSchemas(global_ctx); + } + catch (Exception & e) + { + if (e.code() == ErrorCodes::FAIL_POINT_ERROR) + { + return; + } + else + { + throw; + } + } + } + + void resetSchemas() + { + auto & flash_ctx = global_ctx.getTMTContext(); + flash_ctx.getSchemaSyncer()->reset(); + } + + ManageableStoragePtr mustGetSyncedTable(TableID table_id) + { + auto & flash_ctx = global_ctx.getTMTContext(); + auto & flash_storages = flash_ctx.getStorages(); + auto tbl = flash_storages.get(table_id); + RUNTIME_CHECK(tbl, "Can not find table in TiFlash instance!", table_id); + return tbl; + } + + ManageableStoragePtr mustGetSyncedTableByName(const String & db_name, const String & tbl_name) + { + auto & flash_ctx = global_ctx.getTMTContext(); + auto & flash_storages = flash_ctx.getStorages(); + auto mock_tbl = MockTiDB::instance().getTableByName(db_name, tbl_name); + auto tbl = flash_storages.get(mock_tbl->id()); + RUNTIME_CHECK(tbl, "Can not find table in TiFlash instance!", db_name, tbl_name); + return tbl; + } + + void mustDropSyncedTable(const String & db_idname, const String & tbl_idname) + { + auto drop_query = std::make_shared(); + drop_query->database = db_idname; + drop_query->table = tbl_idname; + drop_query->if_exists = false; + ASTPtr ast_drop_query = drop_query; + InterpreterDropQuery drop_interpreter(ast_drop_query, global_ctx); + drop_interpreter.execute(); + } + + void mustDropSyncedDatabase(const String & db_idname) + { + auto drop_query = std::make_shared(); + drop_query->database = db_idname; + drop_query->if_exists = false; + InterpreterDropQuery drop_interpreter(drop_query, global_ctx); + drop_interpreter.execute(); + } + +private: + static void recreateMetadataPath() + { + String path = TiFlashTestEnv::getContext().getPath(); + auto p = path + "/metadata/"; + TiFlashTestEnv::tryRemovePath(p, /*recreate=*/true); + p = path + "/data/"; + TiFlashTestEnv::tryRemovePath(p, /*recreate=*/true); + } + +protected: + Context & global_ctx; +}; + +TEST_F(SchemaSyncTest, RenameTables) +try +{ + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + MockTiDB::instance().newDataBase(db_name); + + auto cols = ColumnsDescription({ + {"col1", typeFromString("String")}, + {"col2", typeFromString("Int64")}, + }); + // table_name, cols, pk_name + std::vector> tables{ + {"t1", cols, ""}, + {"t2", cols, ""}, + }; + MockTiDB::instance().newTables(db_name, tables, pd_client->getTS(), "dt"); + + refreshSchema(); + + TableID t1_id = mustGetSyncedTableByName(db_name, "t1")->getTableInfo().id; + TableID t2_id = mustGetSyncedTableByName(db_name, "t2")->getTableInfo().id; + + // database_name, table_name, new_table_name + std::vector> table_rename_map{ + {db_name, "t1", "r1"}, + {db_name, "t2", "r2"}, + }; + MockTiDB::instance().renameTables(table_rename_map); + + refreshSchema(); + + ASSERT_EQ(mustGetSyncedTable(t1_id)->getTableInfo().name, "r1"); + ASSERT_EQ(mustGetSyncedTable(t2_id)->getTableInfo().name, "r2"); +} +CATCH + +TEST_F(SchemaSyncTest, RenamePartitionTable) +try +{ + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + const String tbl_name = "mock_part_tbl"; + + auto cols = ColumnsDescription({ + {"col1", typeFromString("String")}, + {"col2", typeFromString("Int64")}, + }); + + MockTiDB::instance().newDataBase(db_name); + auto logical_table_id = MockTiDB::instance().newTable(db_name, tbl_name, cols, pd_client->getTS(), "", "dt"); + auto part1_id = MockTiDB::instance().newPartition(logical_table_id, "red", pd_client->getTS(), /*is_add_part*/ true); + auto part2_id = MockTiDB::instance().newPartition(logical_table_id, "blue", pd_client->getTS(), /*is_add_part*/ true); + + // TODO: write some data + + refreshSchema(); + + // check partition table are created + // TODO: read from partition table + { + auto logical_tbl = mustGetSyncedTable(logical_table_id); + ASSERT_EQ(logical_tbl->getTableInfo().name, tbl_name); + auto part1_tbl = mustGetSyncedTable(part1_id); + ASSERT_EQ(part1_tbl->getTableInfo().name, fmt::format("t_{}", part1_id)); + auto part2_tbl = mustGetSyncedTable(part2_id); + ASSERT_EQ(part2_tbl->getTableInfo().name, fmt::format("t_{}", part2_id)); + } + + const String new_tbl_name = "mock_part_tbl_renamed"; + MockTiDB::instance().renameTable(db_name, tbl_name, new_tbl_name); + refreshSchema(); + + // check partition table are renamed + // TODO: read from renamed partition table + { + auto logical_tbl = mustGetSyncedTable(logical_table_id); + ASSERT_EQ(logical_tbl->getTableInfo().name, new_tbl_name); + auto part1_tbl = mustGetSyncedTable(part1_id); + ASSERT_EQ(part1_tbl->getTableInfo().name, fmt::format("t_{}", part1_id)); + auto part2_tbl = mustGetSyncedTable(part2_id); + ASSERT_EQ(part2_tbl->getTableInfo().name, fmt::format("t_{}", part2_id)); + } +} +CATCH + +TEST_F(SchemaSyncTest, PartitionTableRestart) +try +{ + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + const String tbl_name = "mock_part_tbl"; + + auto cols = ColumnsDescription({ + {"col_1", typeFromString("String")}, + {"col_2", typeFromString("Int64")}, + }); + + auto db_id = MockTiDB::instance().newDataBase(db_name); + auto logical_table_id = MockTiDB::instance().newTable(db_name, tbl_name, cols, pd_client->getTS(), "", "dt"); + auto part1_id = MockTiDB::instance().newPartition(logical_table_id, "red", pd_client->getTS(), /*is_add_part*/ true); + auto part2_id = MockTiDB::instance().newPartition(logical_table_id, "green", pd_client->getTS(), /*is_add_part*/ true); + auto part3_id = MockTiDB::instance().newPartition(logical_table_id, "blue", pd_client->getTS(), /*is_add_part*/ true); + + refreshSchema(); + { + mustGetSyncedTable(part1_id); + mustGetSyncedTable(part2_id); + mustGetSyncedTable(part3_id); + mustGetSyncedTable(logical_table_id); + } + + // schema syncer guarantees logical table creation at last, so there won't be cases + // that logical table exists whereas physical table not. + // mock that part3 and logical table is not created before restart + { + mustDropSyncedTable(fmt::format("db_{}", db_id), fmt::format("t_{}", part3_id)); + mustDropSyncedTable(fmt::format("db_{}", db_id), fmt::format("t_{}", logical_table_id)); + } + + resetSchemas(); + + // add column + MockTiDB::instance().addColumnToTable(db_name, tbl_name, NameAndTypePair{"col_3", typeFromString("Nullable(Int8)")}, Field{}); + const String new_tbl_name = "mock_part_tbl_1"; + MockTiDB::instance().renameTable(db_name, tbl_name, new_tbl_name); + refreshSchema(); + + { + auto part1_tbl = mustGetSyncedTable(part1_id); + ASSERT_EQ(part1_tbl->getTableInfo().name, fmt::format("t_{}", part1_id)); + auto part2_tbl = mustGetSyncedTable(part2_id); + ASSERT_EQ(part2_tbl->getTableInfo().name, fmt::format("t_{}", part2_id)); + auto part3_tbl = mustGetSyncedTable(part3_id); + ASSERT_EQ(part3_tbl->getTableInfo().name, fmt::format("t_{}", part3_id)); + auto logical_tbl = mustGetSyncedTable(logical_table_id); + ASSERT_EQ(logical_tbl->getTableInfo().name, new_tbl_name); + } + + + // drop one partition + resetSchemas(); + MockTiDB::instance().dropPartition(db_name, new_tbl_name, part1_id); + refreshSchema(); + auto part1_tbl = mustGetSyncedTable(part1_id); + ASSERT_EQ(part1_tbl->isTombstone(), true); +} +CATCH + +} // namespace tests +} // namespace DB diff --git a/tests/delta-merge-test/raft/schema/partition_table_restart.test b/tests/delta-merge-test/raft/schema/partition_table_restart.test deleted file mode 100644 index c7a5e488111..00000000000 --- a/tests/delta-merge-test/raft/schema/partition_table_restart.test +++ /dev/null @@ -1,55 +0,0 @@ -# Copyright 2022 PingCAP, Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -=> DBGInvoke __enable_schema_sync_service('false') - -=> DBGInvoke __drop_tidb_table(default, test) -=> DBGInvoke __refresh_schemas() -=> drop table if exists default.test -=> drop table if exists default.test_9999 -=> drop table if exists default.test_9998 -=> drop table if exists default.test_9997 - -=> DBGInvoke __mock_tidb_table(default, test, 'col_1 String, col_2 Int64', '', 'dt') -=> DBGInvoke __mock_tidb_partition(default, test, 9999) -=> DBGInvoke __mock_tidb_partition(default, test, 9998) -=> DBGInvoke __mock_tidb_partition(default, test, 9997) -=> DBGInvoke __refresh_schemas() - -=> drop table default.test_9997 -# schema syncer guarantees logical table creation at last, so there won't be cases that logical table exists whereas physical table not. -=> drop table default.test -=> DBGInvoke __reset_schemas() - -=> DBGInvoke __add_column_to_tidb_table(default, test, 'col_3 Nullable(Int8)') -=> DBGInvoke __refresh_schemas() -=> select col_2 from default.test_9997 - -=> DBGInvoke __reset_schemas() -=> DBGInvoke __drop_tidb_partition(default, test, 9997) -=> DBGInvoke __refresh_schemas() -=> DBGInvoke is_tombstone(default, test_9997) -┌─is_tombstone(default, test_9997)─┐ -│ true │ -└───────────────────────────────────┘ -=> select * from default.test_9999 - -=> DBGInvoke __drop_tidb_table(default, test) -=> DBGInvoke __refresh_schemas() -=> drop table if exists default.test -=> drop table if exists default.test_9999 -=> drop table if exists default.test_9998 -=> drop table if exists default.test_9997 - -=> DBGInvoke __enable_schema_sync_service('true') From b522a726ad11836d75af92766f2ec4428b203d98 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Thu, 1 Sep 2022 20:07:25 +0800 Subject: [PATCH 07/11] Format files Signed-off-by: JaySon-Huang --- dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index a9ee22baa67..a52fdc9c393 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -16,6 +16,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -27,10 +30,6 @@ #include #include -#include "Interpreters/InterpreterDropQuery.h" -#include "Parsers/ASTDropQuery.h" -#include "Parsers/IAST.h" - namespace DB { namespace FailPoints From 69aba5a5d57e6c56d4d3bb3473bb74f0a548111a Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 2 Sep 2022 17:25:47 +0800 Subject: [PATCH 08/11] Add comment Signed-off-by: JaySon-Huang --- dbms/src/TiDB/Schema/TiDBSchemaSyncer.h | 3 +++ dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp | 13 +++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h b/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h index baabb507f17..2f71f846490 100644 --- a/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h +++ b/dbms/src/TiDB/Schema/TiDBSchemaSyncer.h @@ -74,6 +74,9 @@ struct TiDBSchemaSyncer : public SchemaSyncer } // just for test + // It clear all synced database info and reset the `cur_version` to 0. + // All info will fetch from the `getter` again the next time + // `syncSchemas` is call. void reset() override { std::lock_guard lock(schema_mutex); diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index a52fdc9c393..8acf2c22e62 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -90,6 +90,7 @@ class SchemaSyncTest : public ::testing::Test global_ctx.initializeSchemaSyncService(); } + // Sync schema info from TiDB/MockTiDB to TiFlash void refreshSchema() { auto & flash_ctx = global_ctx.getTMTContext(); @@ -111,31 +112,39 @@ class SchemaSyncTest : public ::testing::Test } } + // Reset the schema syncer to mock TiFlash shutdown void resetSchemas() { auto & flash_ctx = global_ctx.getTMTContext(); flash_ctx.getSchemaSyncer()->reset(); } + // Get the TiFlash synced table ManageableStoragePtr mustGetSyncedTable(TableID table_id) { auto & flash_ctx = global_ctx.getTMTContext(); auto & flash_storages = flash_ctx.getStorages(); auto tbl = flash_storages.get(table_id); - RUNTIME_CHECK(tbl, "Can not find table in TiFlash instance!", table_id); + RUNTIME_CHECK_MSG(tbl, "Can not find table in TiFlash instance! table_id={}", table_id); return tbl; } + // Get the TiFlash synced table + // `db_name`, `tbl_name` is the name from the TiDB-server side ManageableStoragePtr mustGetSyncedTableByName(const String & db_name, const String & tbl_name) { auto & flash_ctx = global_ctx.getTMTContext(); auto & flash_storages = flash_ctx.getStorages(); auto mock_tbl = MockTiDB::instance().getTableByName(db_name, tbl_name); auto tbl = flash_storages.get(mock_tbl->id()); - RUNTIME_CHECK(tbl, "Can not find table in TiFlash instance!", db_name, tbl_name); + RUNTIME_CHECK_MSG(tbl, "Can not find table in TiFlash instance! db_name={}, tbl_name={}", db_name, tbl_name); return tbl; } + /* + * Helper methods work with `db_${database_id}`.`t_${table_id}` in the TiFlash side + */ + void mustDropSyncedTable(const String & db_idname, const String & tbl_idname) { auto drop_query = std::make_shared(); From e7818a08422b4e09f16e499b7a250f2ef3d14f23 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 2 Sep 2022 18:32:31 +0800 Subject: [PATCH 09/11] Fix path for testing Signed-off-by: JaySon-Huang --- dbms/src/TestUtils/TiFlashTestBasic.h | 4 +++ dbms/src/TestUtils/TiFlashTestEnv.cpp | 43 ++++++++++++++++++++++++++- dbms/src/TestUtils/TiFlashTestEnv.h | 32 ++------------------ 3 files changed, 48 insertions(+), 31 deletions(-) diff --git a/dbms/src/TestUtils/TiFlashTestBasic.h b/dbms/src/TestUtils/TiFlashTestBasic.h index 91c2cc1d061..5de50a71424 100644 --- a/dbms/src/TestUtils/TiFlashTestBasic.h +++ b/dbms/src/TestUtils/TiFlashTestBasic.h @@ -73,6 +73,10 @@ namespace tests FAIL(); \ } +/** + * GTest related helper functions + */ + /// helper functions for comparing DataType ::testing::AssertionResult DataTypeCompare( const char * lhs_expr, diff --git a/dbms/src/TestUtils/TiFlashTestEnv.cpp b/dbms/src/TestUtils/TiFlashTestEnv.cpp index be88200c53d..b34abb48b3d 100644 --- a/dbms/src/TestUtils/TiFlashTestEnv.cpp +++ b/dbms/src/TestUtils/TiFlashTestEnv.cpp @@ -28,6 +28,42 @@ namespace DB::tests { std::unique_ptr TiFlashTestEnv::global_context = nullptr; +String TiFlashTestEnv::getTemporaryPath(const std::string_view test_case, bool get_abs) +{ + String path = "./tmp/"; + if (!test_case.empty()) + path += std::string(test_case); + + Poco::Path poco_path(path); + if (get_abs) + return poco_path.absolute().toString(); + else + return poco_path.toString(); +} + +void TiFlashTestEnv::tryRemovePath(const std::string & path, bool recreate) +{ + try + { + // drop the data on disk + Poco::File p(path); + if (p.exists()) + { + p.remove(true); + } + + // re-create empty directory for testing + if (recreate) + { + p.createDirectories(); + } + } + catch (...) + { + tryLogCurrentException("gtest", fmt::format("while removing dir `{}`", path)); + } +} + void TiFlashTestEnv::initializeGlobalContext(Strings testdata_path, PageStorageRunMode ps_run_mode, uint64_t bg_thread_count) { // set itself as global context @@ -95,7 +131,12 @@ Context TiFlashTestEnv::getContext(const DB::Settings & settings, Strings testda Context context = *global_context; context.setGlobalContext(*global_context); // Load `testdata_path` as path if it is set. - const String root_path = testdata_path.empty() ? (DB::toString(getpid()) + "/" + getTemporaryPath()) : testdata_path[0]; + const String root_path = [&]() { + const auto root_path = testdata_path.empty() + ? (DB::toString(getpid()) + "/" + getTemporaryPath("", /*get_abs*/ false)) + : testdata_path[0]; + return Poco::Path(root_path).absolute().toString(); + }(); if (testdata_path.empty()) testdata_path.push_back(root_path); context.setPath(root_path); diff --git a/dbms/src/TestUtils/TiFlashTestEnv.h b/dbms/src/TestUtils/TiFlashTestEnv.h index 636ffa06f95..e599016b87f 100644 --- a/dbms/src/TestUtils/TiFlashTestEnv.h +++ b/dbms/src/TestUtils/TiFlashTestEnv.h @@ -27,37 +27,9 @@ namespace DB::tests class TiFlashTestEnv { public: - static String getTemporaryPath(const std::string_view test_case = "") - { - String path = "./tmp/"; - if (!test_case.empty()) - path += std::string(test_case); - - return Poco::Path(path).absolute().toString(); - } - - static void tryRemovePath(const std::string & path, bool recreate = false) - { - try - { - // drop the data on disk - Poco::File p(path); - if (p.exists()) - { - p.remove(true); - } + static String getTemporaryPath(const std::string_view test_case = "", bool get_abs = true); - // re-create empty directory for testing - if (recreate) - { - p.createDirectories(); - } - } - catch (...) - { - tryLogCurrentException("gtest", fmt::format("while removing dir `{}`", path)); - } - } + static void tryRemovePath(const std::string & path, bool recreate = false); static std::pair getPathPool(const Strings & testdata_path = {}) { From 77c1cdf789a5351dc8e5779cf3e295198cc157e7 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 2 Sep 2022 19:05:12 +0800 Subject: [PATCH 10/11] Remove useless var Signed-off-by: JaySon-Huang --- dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp | 7 +------ dbms/src/Storages/DeltaMerge/DeltaMergeStore.h | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 8564b05cfe2..688525dd817 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -355,13 +355,8 @@ void DeltaMergeStore::setUpBackgroundTask(const DMContextPtr & dm_context) blockable_background_pool_handle->wake(); } -void DeltaMergeStore::rename(String /*new_path*/, bool clean_rename, String new_database_name, String new_table_name) +void DeltaMergeStore::rename(String /*new_path*/, String new_database_name, String new_table_name) { - RUNTIME_ASSERT(clean_rename, - log, - "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", - new_database_name, - new_table_name); path_pool.rename(new_database_name, new_table_name); // TODO: replacing these two variables is not atomic, but could be good enough? diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h index 769786ce070..4574c67b4d8 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.h @@ -314,7 +314,7 @@ class DeltaMergeStore : private boost::noncopyable return table_name; } - void rename(String new_path, bool clean_rename, String new_database_name, String new_table_name); + void rename(String new_path, String new_database_name, String new_table_name); void clearData(); From 3f43f864267da606d64e4d4dedc1e8fa321abeb5 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sat, 3 Sep 2022 01:18:17 +0800 Subject: [PATCH 11/11] fix Signed-off-by: JaySon-Huang --- dbms/src/Storages/StorageDeltaMerge.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/dbms/src/Storages/StorageDeltaMerge.cpp b/dbms/src/Storages/StorageDeltaMerge.cpp index 31d9ded2551..8c9f115a472 100644 --- a/dbms/src/Storages/StorageDeltaMerge.cpp +++ b/dbms/src/Storages/StorageDeltaMerge.cpp @@ -1195,23 +1195,25 @@ void StorageDeltaMerge::rename( const String & new_display_table_name) { tidb_table_info.name = new_display_table_name; // update name in table info - // For DatabaseTiFlash, simply update store's database is OK. - // `store->getTableName() == new_table_name` only keep for mock test. - bool clean_rename = !data_path_contains_database_name && getTableName() == new_table_name; - RUNTIME_ASSERT(clean_rename, - log, - "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", - new_database_name, - new_table_name); + { + // For DatabaseTiFlash, simply update store's database is OK. + // `store->getTableName() == new_table_name` only keep for mock test. + bool clean_rename = !data_path_contains_database_name && getTableName() == new_table_name; + RUNTIME_ASSERT(clean_rename, + log, + "should never rename the directories when renaming table, new_database_name={}, new_table_name={}", + new_database_name, + new_table_name); + } if (storeInited()) { - _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); + _store->rename(new_path_to_db, new_database_name, new_table_name); return; } std::lock_guard lock(store_mutex); if (storeInited()) { - _store->rename(new_path_to_db, clean_rename, new_database_name, new_table_name); + _store->rename(new_path_to_db, new_database_name, new_table_name); } else {