ddl: Fix logging cause by non-dropped table (#8955)

close #8911

dbms/src/TiDB/Schema/SchemaSyncService.cpp

@@ -408,6 +408,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);
@@ -417,6 +419,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
     {
@@ -426,9 +433,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,
             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

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
     {

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