to revert really remove lock

Signed-off-by: CalvinNeo <[email protected]>
pingcap · Jun 19, 2023 · 7db53fe · 7db53fe
1 parent 94c9265
commit 7db53fe
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 6 deletions.
diff --git a/dbms/src/Storages/Transaction/KVStore.cpp b/dbms/src/Storages/Transaction/KVStore.cpp
@@ -950,9 +950,9 @@ void KVStore::compactLogByRowKeyRange(TMTContext & tmt, const DM::RowKeyRange &
         auto region_id = region.first;
         auto region_ptr = std::get<3>(region.second);
         LOG_DEBUG(log, "flush extra segment of region {}, region range:[{},{}], flushed segment range:[{},{}]", region.first, region_rowkey_range.getStart().toDebugString(), region_rowkey_range.getEnd().toDebugString(), rowkey_range.getStart().toDebugString(), rowkey_range.getEnd().toDebugString());
-        auto region_task_lock = region_manager.genRegionTaskLock(region_id);
         // Both flushCache and persistRegion should be protected by region task lock.
         storage->flushCache(tmt.getContext(), std::get<2>(region.second));
+        auto region_task_lock = region_manager.genRegionTaskLock(region_id);
         persistRegion(*region_ptr, std::make_optional(&region_task_lock), "triggerCompactLog");
     }
     // TODO Flush the segments that isn't related to any region. Is it really necessary?
@@ -961,7 +961,7 @@ void KVStore::compactLogByRowKeyRange(TMTContext & tmt, const DM::RowKeyRange &
     watch.restart();
 
     // forbid regions being removed.
-    auto task_lock = genTaskLock();
+    // auto task_lock = genTaskLock();
     for (const auto & region : region_compact_indexes)
     {
         // Can truncated to flushed index, which is applied_index in this case.

diff --git a/dbms/src/Storages/Transaction/Region.cpp b/dbms/src/Storages/Transaction/Region.cpp
@@ -722,16 +722,19 @@ EngineStoreApplyRes Region::handleWriteRaftCmd(const WriteCmdsView & cmds, UInt6
     };
 
     {
-        std::unique_lock<std::shared_mutex> lock(mutex);
-
-        handle_write_cmd_func();
+        {
+            // RegionTable::writeBlockByRegion may lead to persistRegion when flush proactively.
+            // So we can't lock here.
+            std::unique_lock<std::shared_mutex> lock(mutex);
+            handle_write_cmd_func();
+        }
 
         // If transfer-leader happened during ingest-sst, there might be illegal data.
         if (0 != cmds.len)
         {
             /// Flush data right after they are committed.
             RegionDataReadInfoList data_list_to_remove;
-            RegionTable::writeBlockByRegion(context, shared_from_this(), data_list_to_remove, log, false);
+            RegionTable::writeBlockByRegion(context, shared_from_this(), data_list_to_remove, log, true);
         }
 
         meta.setApplied(index, term);