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

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