[FLASH-1028] Region merge should not remove data

dbms/src/Debug/MockTiKV.h

@@ -15,7 +15,12 @@ class MockTiKV : public ext::singleton<MockTiKV>
         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;
     }

dbms/src/Storages/Transaction/KVStore.cpp

@@ -1,6 +1,6 @@
 #include <Interpreters/Context.h>
-#include <Storages/StorageDeltaMergeHelpers.h>
 #include <Storages/StorageDeltaMerge.h>
+#include <Storages/StorageDeltaMergeHelpers.h>
 #include <Storages/Transaction/BackgroundService.h>
 #include <Storages/Transaction/KVStore.h>
 #include <Storages/Transaction/ProxyFFIType.h>
@@ -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;

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(const std::string &)>);
-    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;
 

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<std::mutex> 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.

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();
 

tests/delta-merge-test/raft/region_merge.test

@@ -0,0 +1,107 @@
+=> 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', '', '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)
+=> 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 │
+│    10 │           2 │
+│    14 │           3 │
+│    16 │           5 │
+└───────┴─────────────┘
+=> 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 │
+│    14 │           3 │                 1 │                 0 │
+│    15 │           4 │                 3 │                 0 │
+│     0 │           4 │                 4 │                 1 │
+│    16 │           5 │                 1 │                 0 │
+└───────┴─────────────┴───────────────────┴───────────────────┘
+=> DBGInvoke dump_all_mock_region(default, test)
+┌─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)        │
+└─────────────────────────────────────────────────┘
+=> 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 │
+│    16 │           5 │
+│    19 │          10 │
+│    18 │          11 │
+└───────┴─────────────┘
+=> 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 │
+│    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 │
+│ [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 │
+│ [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 │
+│ 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