From 9002cc34d3b593a718b6c5260ba18f30a45ab314 Mon Sep 17 00:00:00 2001
From: Ti Chi Robot <ti-community-prow-bot@tidb.io>
Date: Thu, 18 Apr 2024 15:18:06 +0800
Subject: [PATCH] ddl: Fix logging cause by non-dropped table (#8955) (#8958)

close pingcap/tiflash#8911
---
 dbms/src/TiDB/Schema/SchemaSyncService.cpp    | 12 +++++++--
 dbms/src/TiDB/Schema/SchemaSyncService.h      |  4 ++-
 .../TiDB/Schema/tests/gtest_schema_sync.cpp   | 26 ++++++++++++++++---
 3 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/dbms/src/TiDB/Schema/SchemaSyncService.cpp b/dbms/src/TiDB/Schema/SchemaSyncService.cpp
index 419a2f28e4a..2cfff9c59e3 100644
--- a/dbms/src/TiDB/Schema/SchemaSyncService.cpp
+++ b/dbms/src/TiDB/Schema/SchemaSyncService.cpp
@@ -411,6 +411,8 @@ bool SchemaSyncService::gcImpl(Timestamp gc_safepoint, KeyspaceID keyspace_id, b
         }
     }
 
+    // TODO: Optimize it after `BackgroundProcessingPool` can the task return how many seconds to sleep
+    //       before next round.
     if (succeeded)
     {
         updateLastGcSafepoint(keyspace_id, gc_safepoint);
@@ -420,6 +422,11 @@ bool SchemaSyncService::gcImpl(Timestamp gc_safepoint, KeyspaceID keyspace_id, b
             num_tables_removed,
             num_databases_removed,
             gc_safepoint);
+        // This round of GC could run for a long time. Run immediately to check whether
+        // the latest gc_safepoint has been updated in PD.
+        // - gc_safepoint is not updated, it will be skipped because gc_safepoint == last_gc_safepoint
+        // - gc_safepoint is updated, run again immediately to cleanup other dropped data
+        return true;
     }
     else
     {
@@ -429,9 +436,10 @@ bool SchemaSyncService::gcImpl(Timestamp gc_safepoint, KeyspaceID keyspace_id, b
             "Schema GC meet error, will try again later, last_safepoint={} safepoint={}",
             last_gc_safepoint_str,
             gc_safepoint);
+        // Return false to let it run again after `ddl_sync_interval_seconds` even if the gc_safepoint
+        // on PD is not updated.
+        return false;
     }
-
-    return true;
 }
 
 } // namespace DB
diff --git a/dbms/src/TiDB/Schema/SchemaSyncService.h b/dbms/src/TiDB/Schema/SchemaSyncService.h
index bd72c904051..e2c0b572c0d 100644
--- a/dbms/src/TiDB/Schema/SchemaSyncService.h
+++ b/dbms/src/TiDB/Schema/SchemaSyncService.h
@@ -28,7 +28,8 @@ namespace DB
 namespace tests
 {
 class RegionKVStoreTest;
-}
+class SchemaSyncTest;
+} // namespace tests
 class Logger;
 using LoggerPtr = std::shared_ptr<Logger>;
 
@@ -65,6 +66,7 @@ class SchemaSyncService
 
     friend void dbgFuncGcSchemas(Context &, const ASTs &, DBGInvokerPrinter);
     friend class tests::RegionKVStoreTest;
+    friend class tests::SchemaSyncTest;
 
     struct KeyspaceGCContext
     {
diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp
index c5dc2648849..ba24eab316f 100644
--- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp
+++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp
@@ -44,7 +44,8 @@ extern const char exception_before_rename_table_old_meta_removed[];
 extern const char force_context_path[];
 extern const char force_set_num_regions_for_table[];
 } // namespace FailPoints
-namespace tests
+} // namespace DB
+namespace DB::tests
 {
 class SchemaSyncTest : public ::testing::Test
 {
@@ -189,6 +190,11 @@ class SchemaSyncTest : public ::testing::Test
         drop_interpreter.execute();
     }
 
+    static std::optional<Timestamp> lastGcSafePoint(const SchemaSyncServicePtr & sync_service, KeyspaceID keyspace_id)
+    {
+        return sync_service->lastGcSafePoint(keyspace_id);
+    }
+
 private:
     static void recreateMetadataPath()
     {
@@ -352,8 +358,21 @@ try
     SCOPE_EXIT({ FailPointHelper::disableFailPoint(FailPoints::force_set_num_regions_for_table); });
 
     auto sync_service = std::make_shared<SchemaSyncService>(global_ctx);
-    ASSERT_TRUE(sync_service->gc(std::numeric_limits<UInt64>::max(), NullspaceID));
+    {
+        // ensure gc_safe_point cache is empty
+        auto last_gc_safe_point = lastGcSafePoint(sync_service, NullspaceID);
+        ASSERT_FALSE(last_gc_safe_point.has_value());
+    }
 
+    // Run GC, but the table is not physically dropped because `force_set_num_regions_for_table`
+    ASSERT_FALSE(sync_service->gc(std::numeric_limits<UInt64>::max(), NullspaceID));
+    {
+        // gc_safe_point cache is not updated
+        auto last_gc_safe_point = lastGcSafePoint(sync_service, NullspaceID);
+        ASSERT_FALSE(last_gc_safe_point.has_value());
+    }
+
+    // ensure the table is not physically dropped
     size_t num_remain_tables = 0;
     for (auto table_id : table_ids)
     {
@@ -503,5 +522,4 @@ try
 }
 CATCH
 
-} // namespace tests
-} // namespace DB
+} // namespace DB::tests