diff --git a/dbms/src/TiDB/Schema/SchemaSyncService.cpp b/dbms/src/TiDB/Schema/SchemaSyncService.cpp index 4a7a263da36..884823c0483 100644 --- a/dbms/src/TiDB/Schema/SchemaSyncService.cpp +++ b/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 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; @@ -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 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(global_ctx); - ASSERT_TRUE(sync_service->gc(std::numeric_limits::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::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