From 3e0776583b4f4daa29a1eac954650b6045934b96 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 Mar 2020 14:23:37 +0800 Subject: [PATCH 1/5] Remove region after region merge should not remove data Signed-off-by: JaySon-Huang --- dbms/src/Storages/Transaction/KVStore.cpp | 25 ++++++++++++------- dbms/src/Storages/Transaction/KVStore.h | 11 ++++++-- dbms/src/Storages/Transaction/RegionTable.cpp | 5 +++- dbms/src/Storages/Transaction/RegionTable.h | 2 +- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index af5810caf11..7a38bf68de0 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -1,6 +1,6 @@ #include -#include #include +#include #include #include #include @@ -198,11 +198,11 @@ void KVStore::mockRemoveRegion(const DB::RegionID region_id, RegionTable & regio { auto task_lock = genTaskLock(); auto region_lock = region_manager.genRegionTaskLock(region_id); - removeRegion(region_id, region_table, task_lock, region_lock); + removeRegion(region_id, /* remove_data */ false, region_table, task_lock, region_lock); } -void KVStore::removeRegion( - const RegionID region_id, RegionTable & region_table, const KVStoreTaskLock & task_lock, const RegionTaskLock & region_lock) +void KVStore::removeRegion(const RegionID region_id, bool remove_data, RegionTable & region_table, const KVStoreTaskLock & task_lock, + const RegionTaskLock & region_lock) { LOG_INFO(log, "Start to remove [region " << region_id << "]"); @@ -220,7 +220,7 @@ void KVStore::removeRegion( region_persister.drop(region_id, region_lock); - region_table.removeRegion(region_id); + region_table.removeRegion(region_id, remove_data); LOG_INFO(log, "Remove [region " << region_id << "] done"); } @@ -323,7 +323,7 @@ void KVStore::handleDestroy(UInt64 region_id, TMTContext & tmt) } LOG_INFO(log, "Handle destroy " << region->toString()); region->setPendingRemove(); - removeRegion(region_id, tmt.getRegionTable(), task_lock, region_manager.genRegionTaskLock(region_id)); + removeRegion(region_id, /* remove_data */ true, tmt.getRegionTable(), task_lock, region_manager.genRegionTaskLock(region_id)); } void KVStore::setRegionCompactLogPeriod(Seconds period) { REGION_COMPACT_LOG_PERIOD = period; } @@ -457,7 +457,10 @@ TiFlashApplyRes KVStore::handleAdminRaftCmd(raft_cmdpb::AdminRequest && request, const auto handle_change_peer = [&]() { if (curr_region.isPendingRemove()) - removeRegion(curr_region_id, region_table, task_lock, region_task_lock); + { + // remove `curr_region` from this node, we can remove its data. + removeRegion(curr_region_id, /* remove_data */ true, region_table, task_lock, region_task_lock); + } else persist_and_sync(); }; @@ -470,7 +473,9 @@ TiFlashApplyRes KVStore::handleAdminRaftCmd(raft_cmdpb::AdminRequest && request, { auto source_region = getRegion(source_region_id); source_region->setPendingRemove(); - removeRegion(source_region_id, region_table, task_lock, region_manager.genRegionTaskLock(source_region_id)); + // `source_region` is merged, don't remove its data in storage. + removeRegion( + source_region_id, /* remove_data */ false, region_table, task_lock, region_manager.genRegionTaskLock(source_region_id)); } region_range_index.remove(result.ori_region_range->comparableKeys(), curr_region_id); region_range_index.add(curr_region_ptr); @@ -489,7 +494,9 @@ TiFlashApplyRes KVStore::handleAdminRaftCmd(raft_cmdpb::AdminRequest && request, "Admin cmd " << raft_cmdpb::AdminCmdType_Name(type) << " has been applied, try to remove source " << source_region->toString(false)); source_region->setPendingRemove(); - removeRegion(source_region->id(), region_table, task_lock, region_manager.genRegionTaskLock(source_region->id())); + // `source_region` is merged, don't remove its data in storage. + removeRegion(source_region->id(), /* remove_data */ false, region_table, task_lock, + region_manager.genRegionTaskLock(source_region->id())); } } break; diff --git a/dbms/src/Storages/Transaction/KVStore.h b/dbms/src/Storages/Transaction/KVStore.h index 0a80400d143..b2063681983 100644 --- a/dbms/src/Storages/Transaction/KVStore.h +++ b/dbms/src/Storages/Transaction/KVStore.h @@ -80,8 +80,15 @@ class KVStore final : private boost::noncopyable friend class MockTiDB; friend struct MockTiDBTable; friend void dbgFuncRemoveRegion(Context &, const ASTs &, /*DBGInvoker::Printer*/ std::function); - void removeRegion( - const RegionID region_id, RegionTable & region_table, const KVStoreTaskLock & task_lock, const RegionTaskLock & region_lock); + + // Remove region from this TiFlash node. + // If region is destroy or moved to another node(change peer), + // set `remove_data` true to remove obsolete data from storage. + void removeRegion(const RegionID region_id, + bool remove_data, + RegionTable & region_table, + const KVStoreTaskLock & task_lock, + const RegionTaskLock & region_lock); void mockRemoveRegion(const RegionID region_id, RegionTable & region_table); KVStoreTaskLock genTaskLock() const; diff --git a/dbms/src/Storages/Transaction/RegionTable.cpp b/dbms/src/Storages/Transaction/RegionTable.cpp index d5ab53cbb65..64c2ea173ed 100644 --- a/dbms/src/Storages/Transaction/RegionTable.cpp +++ b/dbms/src/Storages/Transaction/RegionTable.cpp @@ -230,7 +230,7 @@ TableID RegionTable::popOneTableToOptimize() return res; } -void RegionTable::removeRegion(const RegionID region_id) +void RegionTable::removeRegion(const RegionID region_id, bool remove_data) { std::lock_guard lock(mutex); @@ -246,6 +246,9 @@ void RegionTable::removeRegion(const RegionID region_id) do { + // Sometime we don't need to remove data. e.g. remove region after region merge. + if (!remove_data) + break; /// Some region of this table is removed, if it is a DeltaTree, write deleteRange. /// Now we assume that StorageDeltaMerge::deleteRange do not block for long time and do it in sync mode. diff --git a/dbms/src/Storages/Transaction/RegionTable.h b/dbms/src/Storages/Transaction/RegionTable.h index 55361b649bd..ed7e1874976 100644 --- a/dbms/src/Storages/Transaction/RegionTable.h +++ b/dbms/src/Storages/Transaction/RegionTable.h @@ -108,7 +108,7 @@ class RegionTable : private boost::noncopyable /// This functional only shrink the table range of this region_id void shrinkRegionRange(const Region & region); - void removeRegion(const RegionID region_id); + void removeRegion(const RegionID region_id, bool remove_data); TableID popOneTableToOptimize(); From 6290c19363f383318e05e850c2b8f3da8824ed14 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 Mar 2020 14:25:13 +0800 Subject: [PATCH 2/5] Fix region init index Signed-off-by: JaySon-Huang --- dbms/src/Debug/MockTiKV.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dbms/src/Debug/MockTiKV.h b/dbms/src/Debug/MockTiKV.h index 443cc1705bf..4a3e054fe6d 100644 --- a/dbms/src/Debug/MockTiKV.h +++ b/dbms/src/Debug/MockTiKV.h @@ -15,7 +15,12 @@ class MockTiKV : public ext::singleton std::lock_guard lock(mutex); auto it = raft_index.find(region_id); if (it == raft_index.end()) - it = raft_index.emplace_hint(it, region_id, RAFT_INIT_LOG_INDEX); + { + // Usually index 6 is empty and we ignore it. + // https://github.com/tikv/tikv/issues/7047 + auto init_index = RAFT_INIT_LOG_INDEX + 1; + it = raft_index.emplace_hint(it, region_id, init_index); + } ++(it->second); return it->second; } From afc7d1320952d767ae14ca59b8c50b77370fe355 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 Mar 2020 14:27:21 +0800 Subject: [PATCH 3/5] Add region_merge.test for DT Signed-off-by: JaySon-Huang --- tests/delta-merge-test/raft/region_merge.test | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) create mode 100644 tests/delta-merge-test/raft/region_merge.test diff --git a/tests/delta-merge-test/raft/region_merge.test b/tests/delta-merge-test/raft/region_merge.test new file mode 100644 index 00000000000..26c149c656c --- /dev/null +++ b/tests/delta-merge-test/raft/region_merge.test @@ -0,0 +1,115 @@ +=> DBGInvoke __enable_schema_sync_service('true') + +=> DBGInvoke __drop_tidb_table(default, test) +=> drop table if exists default.test + +=> DBGInvoke __mock_tidb_table(default, test, 'col_1 Int64') + +=> DBGInvoke __region_snapshot(4, 0, 100, default, test) + +=> DBGInvoke __raft_insert_row_full(default, test, 4, 2, 1, 0, 10) +=> DBGInvoke __raft_insert_row_full(default, test, 4, 1, 1, 0, 12) +=> DBGInvoke __raft_insert_row_full(default, test, 4, 3, 1, 0, 14) +=> DBGInvoke __raft_insert_row_full(default, test, 4, 4, 4, 1, 0) +=> DBGInvoke __raft_insert_row_full(default, test, 4, 4, 3, 0, 15) +=> DBGInvoke __raft_insert_row_full(default, test, 4, 5, 1, 0, 16) +=> select (*) from default.test " --schema_version "10000000 +┌─col_1─┬─_tidb_rowid─┐ +│ 12 │ 1 │ +└───────┴─────────────┘ +┌─col_1─┬─_tidb_rowid─┐ +│ 10 │ 2 │ +│ 14 │ 3 │ +│ 16 │ 5 │ +└───────┴─────────────┘ +=> selraw (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK +┌─col_1─┬─_tidb_rowid─┬─_INTERNAL_VERSION─┬─_INTERNAL_DELMARK─┐ +│ 12 │ 1 │ 1 │ 0 │ +│ 10 │ 2 │ 1 │ 0 │ +│ 14 │ 3 │ 1 │ 0 │ +│ 15 │ 4 │ 3 │ 0 │ +│ 0 │ 4 │ 4 │ 1 │ +│ 16 │ 5 │ 1 │ 0 │ +└───────┴─────────────┴───────────────────┴───────────────────┘ +=> selraw nokvstore (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK +=> DBGInvoke dump_all_mock_region(default, test) +┌─dump_all_mock_region(default, test)────────────────────┐ +│ [region 4] ranges: [0, 100), state: Normal, [write 6 ] │ +│ total size: 1 │ +└────────────────────────────────────────────────────────┘ +=> DBGInvoke region_split(4, default, test, 0, 4, 4, 100, 5) +┌─region_split(4, default, test, 0, 4, 4, 100, 5)─┐ +│ execute batch split, region 4 into (4,5) │ +└─────────────────────────────────────────────────┘ +=> DBGInvoke __raft_insert_row_full(default, test, 5, 10, 1, 0, 17) +=> DBGInvoke __raft_insert_row_full(default, test, 5, 11, 1, 0, 18) +=> DBGInvoke __raft_insert_row_full(default, test, 5, 10, 3, 0, 19) +=> select (*) from default.test +┌─col_1─┬─_tidb_rowid─┐ +│ 12 │ 1 │ +│ 10 │ 2 │ +│ 14 │ 3 │ +└───────┴─────────────┘ +┌─col_1─┬─_tidb_rowid─┐ +│ 16 │ 5 │ +└───────┴─────────────┘ +┌─col_1─┬─_tidb_rowid─┐ +│ 19 │ 10 │ +└───────┴─────────────┘ +┌─col_1─┬─_tidb_rowid─┐ +│ 18 │ 11 │ +└───────┴─────────────┘ +=> selraw (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK +┌─col_1─┬─_tidb_rowid─┬─_INTERNAL_VERSION─┬─_INTERNAL_DELMARK─┐ +│ 12 │ 1 │ 1 │ 0 │ +│ 10 │ 2 │ 1 │ 0 │ +│ 14 │ 3 │ 1 │ 0 │ +│ 15 │ 4 │ 3 │ 0 │ +│ 0 │ 4 │ 4 │ 1 │ +│ 16 │ 5 │ 1 │ 0 │ +│ 17 │ 10 │ 1 │ 0 │ +│ 19 │ 10 │ 3 │ 0 │ +│ 18 │ 11 │ 1 │ 0 │ +└───────┴─────────────┴───────────────────┴───────────────────┘ +=> DBGInvoke dump_all_mock_region(default, test) +┌─dump_all_mock_region(default, test)────────────────────┐ +│ [region 5] ranges: [4, 100), state: Normal, [write 6 ] │ +│ [region 4] ranges: [0, 4), state: Normal, [write 3 ] │ +│ total size: 2 │ +└────────────────────────────────────────────────────────┘ +=> DBGInvoke region_prepare_merge(4, 5); +┌─region_prepare_merge(4, 5)───────────────┐ +│ execute prepare merge, source 4 target 5 │ +└──────────────────────────────────────────┘ +=> DBGInvoke dump_all_mock_region(default, test) +┌─dump_all_mock_region(default, test)────────────────────┐ +│ [region 5] ranges: [4, 100), state: Normal, [write 6 ] │ +│ [region 4] ranges: [0, 4), state: Merging, [write 3 ] │ +│ total size: 2 │ +└────────────────────────────────────────────────────────┘ +=> DBGInvoke region_commit_merge(4, 5); +┌─region_commit_merge(4, 5)────────────────┐ +│ execute commit merge, source 4 current 5 │ +└──────────────────────────────────────────┘ +=> DBGInvoke dump_all_mock_region(default, test) +┌─dump_all_mock_region(default, test)────────────────────┐ +│ [region 5] ranges: [0, 100), state: Normal, [write 9 ] │ +│ total size: 1 │ +└────────────────────────────────────────────────────────┘ +=> DBGInvoke __try_flush_region(5) +=> DBGInvoke dump_all_mock_region(default, test) +┌─dump_all_mock_region(default, test)────────┐ +│ [region 5] ranges: [0, 100), state: Normal │ +│ total size: 1 │ +└────────────────────────────────────────────┘ +=> select _tidb_rowid, col_1 from default.test order by _tidb_rowid +┌─_tidb_rowid─┬─col_1─┐ +│ 1 │ 12 │ +│ 2 │ 10 │ +│ 3 │ 14 │ +│ 5 │ 16 │ +│ 10 │ 19 │ +│ 11 │ 18 │ +└─────────────┴───────┘ +=> DBGInvoke __drop_tidb_table(default, test) +=> drop table if exists default.test From abac442b24e8e7cb1dc2de5d3be0bfea8620d616 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 Mar 2020 14:29:40 +0800 Subject: [PATCH 4/5] Fix different behavior between DT and TMT Signed-off-by: JaySon-Huang --- tests/delta-merge-test/raft/region_merge.test | 52 ++++++++----------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/tests/delta-merge-test/raft/region_merge.test b/tests/delta-merge-test/raft/region_merge.test index 26c149c656c..f117f364222 100644 --- a/tests/delta-merge-test/raft/region_merge.test +++ b/tests/delta-merge-test/raft/region_merge.test @@ -3,10 +3,11 @@ => DBGInvoke __drop_tidb_table(default, test) => drop table if exists default.test -=> DBGInvoke __mock_tidb_table(default, test, 'col_1 Int64') +=> DBGInvoke __mock_tidb_table(default, test, 'col_1 Int64', '', 'dt') => DBGInvoke __region_snapshot(4, 0, 100, default, test) +# raft_insert_row_full(database_name, table_name, region_id, handle_id, tso, del, val1, val2, ...) => DBGInvoke __raft_insert_row_full(default, test, 4, 2, 1, 0, 10) => DBGInvoke __raft_insert_row_full(default, test, 4, 1, 1, 0, 12) => DBGInvoke __raft_insert_row_full(default, test, 4, 3, 1, 0, 14) @@ -16,13 +17,11 @@ => select (*) from default.test " --schema_version "10000000 ┌─col_1─┬─_tidb_rowid─┐ │ 12 │ 1 │ -└───────┴─────────────┘ -┌─col_1─┬─_tidb_rowid─┐ │ 10 │ 2 │ │ 14 │ 3 │ │ 16 │ 5 │ └───────┴─────────────┘ -=> selraw (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK +=> selraw (*),_INTERNAL_VERSION, _INTERNAL_DELMARK from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK ┌─col_1─┬─_tidb_rowid─┬─_INTERNAL_VERSION─┬─_INTERNAL_DELMARK─┐ │ 12 │ 1 │ 1 │ 0 │ │ 10 │ 2 │ 1 │ 0 │ @@ -31,12 +30,11 @@ │ 0 │ 4 │ 4 │ 1 │ │ 16 │ 5 │ 1 │ 0 │ └───────┴─────────────┴───────────────────┴───────────────────┘ -=> selraw nokvstore (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK => DBGInvoke dump_all_mock_region(default, test) -┌─dump_all_mock_region(default, test)────────────────────┐ -│ [region 4] ranges: [0, 100), state: Normal, [write 6 ] │ -│ total size: 1 │ -└────────────────────────────────────────────────────────┘ +┌─dump_all_mock_region(default, test)────────┐ +│ [region 4] ranges: [0, 100), state: Normal │ +│ total size: 1 │ +└────────────────────────────────────────────┘ => DBGInvoke region_split(4, default, test, 0, 4, 4, 100, 5) ┌─region_split(4, default, test, 0, 4, 4, 100, 5)─┐ │ execute batch split, region 4 into (4,5) │ @@ -49,17 +47,11 @@ │ 12 │ 1 │ │ 10 │ 2 │ │ 14 │ 3 │ -└───────┴─────────────┘ -┌─col_1─┬─_tidb_rowid─┐ │ 16 │ 5 │ -└───────┴─────────────┘ -┌─col_1─┬─_tidb_rowid─┐ │ 19 │ 10 │ -└───────┴─────────────┘ -┌─col_1─┬─_tidb_rowid─┐ │ 18 │ 11 │ └───────┴─────────────┘ -=> selraw (*) from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK +=> selraw (*), _INTERNAL_VERSION, _INTERNAL_DELMARK from default.test order by _tidb_rowid, _INTERNAL_VERSION, _INTERNAL_DELMARK ┌─col_1─┬─_tidb_rowid─┬─_INTERNAL_VERSION─┬─_INTERNAL_DELMARK─┐ │ 12 │ 1 │ 1 │ 0 │ │ 10 │ 2 │ 1 │ 0 │ @@ -72,30 +64,30 @@ │ 18 │ 11 │ 1 │ 0 │ └───────┴─────────────┴───────────────────┴───────────────────┘ => DBGInvoke dump_all_mock_region(default, test) -┌─dump_all_mock_region(default, test)────────────────────┐ -│ [region 5] ranges: [4, 100), state: Normal, [write 6 ] │ -│ [region 4] ranges: [0, 4), state: Normal, [write 3 ] │ -│ total size: 2 │ -└────────────────────────────────────────────────────────┘ +┌─dump_all_mock_region(default, test)────────┐ +│ [region 5] ranges: [4, 100), state: Normal │ +│ [region 4] ranges: [0, 4), state: Normal │ +│ total size: 2 │ +└────────────────────────────────────────────┘ => DBGInvoke region_prepare_merge(4, 5); ┌─region_prepare_merge(4, 5)───────────────┐ │ execute prepare merge, source 4 target 5 │ └──────────────────────────────────────────┘ => DBGInvoke dump_all_mock_region(default, test) -┌─dump_all_mock_region(default, test)────────────────────┐ -│ [region 5] ranges: [4, 100), state: Normal, [write 6 ] │ -│ [region 4] ranges: [0, 4), state: Merging, [write 3 ] │ -│ total size: 2 │ -└────────────────────────────────────────────────────────┘ +┌─dump_all_mock_region(default, test)────────┐ +│ [region 5] ranges: [4, 100), state: Normal │ +│ [region 4] ranges: [0, 4), state: Merging │ +│ total size: 2 │ +└────────────────────────────────────────────┘ => DBGInvoke region_commit_merge(4, 5); ┌─region_commit_merge(4, 5)────────────────┐ │ execute commit merge, source 4 current 5 │ └──────────────────────────────────────────┘ => DBGInvoke dump_all_mock_region(default, test) -┌─dump_all_mock_region(default, test)────────────────────┐ -│ [region 5] ranges: [0, 100), state: Normal, [write 9 ] │ -│ total size: 1 │ -└────────────────────────────────────────────────────────┘ +┌─dump_all_mock_region(default, test)────────┐ +│ [region 5] ranges: [0, 100), state: Normal │ +│ total size: 1 │ +└────────────────────────────────────────────┘ => DBGInvoke __try_flush_region(5) => DBGInvoke dump_all_mock_region(default, test) ┌─dump_all_mock_region(default, test)────────┐ From 554a5a0967ad9d66a8a0c1a3f5bc3b2954a649e0 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 24 Mar 2020 01:56:47 +0800 Subject: [PATCH 5/5] Fix mock remove region Signed-off-by: JaySon-Huang --- dbms/src/Storages/Transaction/KVStore.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp index 7a38bf68de0..247b39524fb 100644 --- a/dbms/src/Storages/Transaction/KVStore.cpp +++ b/dbms/src/Storages/Transaction/KVStore.cpp @@ -198,7 +198,8 @@ void KVStore::mockRemoveRegion(const DB::RegionID region_id, RegionTable & regio { auto task_lock = genTaskLock(); auto region_lock = region_manager.genRegionTaskLock(region_id); - removeRegion(region_id, /* remove_data */ false, region_table, task_lock, region_lock); + // mock remove region should remove data by default + removeRegion(region_id, /* remove_data */ true, region_table, task_lock, region_lock); } void KVStore::removeRegion(const RegionID region_id, bool remove_data, RegionTable & region_table, const KVStoreTaskLock & task_lock,