From 2047ad47fdb4df7c88337934307cba412b88967b Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 15 Jul 2024 14:13:29 +0800 Subject: [PATCH 1/7] Add ut --- .../TiDB/Schema/tests/gtest_schema_sync.cpp | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp index 7bdb4308ab3..c0613e82696 100644 --- a/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp +++ b/dbms/src/TiDB/Schema/tests/gtest_schema_sync.cpp @@ -37,6 +37,7 @@ namespace DB namespace FailPoints { extern const char exception_before_rename_table_old_meta_removed[]; +extern const char force_schema_sync_too_old_schema[]; extern const char force_context_path[]; } // namespace FailPoints namespace tests @@ -343,5 +344,110 @@ try } CATCH +TEST_F(SchemaSyncTest, DropTableTombstoneTSCase1) +try +{ + // tiflash/issues/9227 + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + const String tbl_name = "mock_part_tbl"; + + auto cols = ColumnsDescription({ + {"col_1", typeFromString("String")}, + {"col_2", typeFromString("Int64")}, + }); + + /*auto db_id =*/MockTiDB::instance().newDataBase(db_name); + auto logical_table_id = MockTiDB::instance().newTable(db_name, tbl_name, cols, pd_client->getTS(), "", "dt"); + + refreshSchema(); + { + auto storage = mustGetSyncedTable(logical_table_id); + ASSERT_EQ(storage->getTombstone(), 0); + } + + // drop the table + LOG_INFO(Logger::get(), "mock drop table at tidb"); + MockTiDB::instance().dropTable(global_ctx, db_name, tbl_name, /*drop_regions*/ true); + refreshSchema(); + UInt64 tombstone_ts_before_sync_all = 0; + { + // the table should mark as tombstone != 0 + auto storage = mustGetSyncedTable(logical_table_id); + tombstone_ts_before_sync_all = storage->getTombstone(); + ASSERT_NE(tombstone_ts_before_sync_all, 0); + } + LOG_INFO(Logger::get(), "mock drop table at tidb done"); + + LOG_INFO(Logger::get(), "mock tiflash sync all schema"); + FailPointHelper::enableFailPoint(FailPoints::force_schema_sync_too_old_schema); + SCOPE_EXIT({ + FailPointHelper::disableFailPoint(FailPoints::force_schema_sync_too_old_schema); + }); + MockTiDB::instance().newDataBase(db_name + "_copy"); + refreshSchema(); + { + // the table should mark as tombstone != 0 + auto storage = mustGetSyncedTable(logical_table_id); + ASSERT_NE(storage->getTombstone(), 0); + // the tombstone_ts should not changed + ASSERT_EQ(storage->getTombstone(), tombstone_ts_before_sync_all); + } +} +CATCH + +TEST_F(SchemaSyncTest, DropTableTombstoneTSCase2) +try +{ + // tiflash/issues/9227 + auto pd_client = global_ctx.getTMTContext().getPDClient(); + + const String db_name = "mock_db"; + const String tbl_name = "mock_part_tbl"; + + auto cols = ColumnsDescription({ + {"col_1", typeFromString("String")}, + {"col_2", typeFromString("Int64")}, + }); + + /*auto db_id =*/MockTiDB::instance().newDataBase(db_name); + auto logical_table_id = MockTiDB::instance().newTable(db_name, tbl_name, cols, pd_client->getTS(), "", "dt"); + + refreshSchema(); + { + auto storage = mustGetSyncedTable(logical_table_id); + ASSERT_EQ(storage->getTombstone(), 0); + } + + // drop the table during sync all schema + LOG_INFO(Logger::get(), "mock tiflash sync all schema"); + MockTiDB::instance().dropTable(global_ctx, db_name, tbl_name, /*drop_regions*/ true); + FailPointHelper::enableFailPoint(FailPoints::force_schema_sync_too_old_schema); + SCOPE_EXIT({ + FailPointHelper::disableFailPoint(FailPoints::force_schema_sync_too_old_schema); + }); + refreshSchema(); + UInt64 tombstone_ts_by_sync_all = 0; + { + // the table should mark as tombstone != 0 + auto storage = mustGetSyncedTable(logical_table_id); + tombstone_ts_by_sync_all = storage->getTombstone(); + ASSERT_NE(tombstone_ts_by_sync_all, 0); + } + + // trigger sync all schema again, the tombstone_ts should not change + MockTiDB::instance().newDataBase(db_name + "_copy"); + refreshSchema(); + { + // the table should mark as tombstone != 0 + auto storage = mustGetSyncedTable(logical_table_id); + ASSERT_NE(storage->getTombstone(), 0); + // the tombstone_ts should not changed + ASSERT_EQ(storage->getTombstone(), tombstone_ts_by_sync_all); + } +} +CATCH + } // namespace tests } // namespace DB From a2e4d3927cd761b6dee29d99658e03edf604a845 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 12 Jul 2024 16:53:16 +0800 Subject: [PATCH 2/7] Skip updating the tombstone_ts if the table is already tombstone. Signed-off-by: JaySon-Huang --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index d2ae9241128..039f818a8c0 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1285,8 +1285,16 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db LOG_DEBUG(log, "table {} does not exist.", table_id); return; } + + // Skip updating the tombstone_ts if the table is already tombstone. + if (auto tombstone_ts = storage->getTombstone(); tombstone_ts != 0) + { + LOG_INFO(log, "Tombstone table {}.{} has been done before, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); + return; + } + GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); - LOG_INFO(log, "Tombstoning table {}.{}", db_name, name_mapper.debugTableName(storage->getTableInfo())); + LOG_INFO(log, "Tombstone table {}.{} begin", db_name, name_mapper.debugTableName(storage->getTableInfo())); const UInt64 tombstone_ts = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime); AlterCommands commands; { @@ -1302,7 +1310,7 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db } auto alter_lock = storage->lockForAlter(getThreadNameAndID()); storage->alterFromTiDB(alter_lock, commands, db_name, storage->getTableInfo(), name_mapper, context); - LOG_INFO(log, "Tombstoned table {}.{}, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); + LOG_INFO(log, "Tombstone table {}.{} end, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); } template From 952c552248293b45f9703a9f79825f8454d30a86 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Sun, 14 Jul 2024 13:59:29 +0800 Subject: [PATCH 3/7] Refine logging Signed-off-by: JaySon-Huang --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 97 ++++++++++++++------------ dbms/src/TiDB/Schema/SchemaBuilder.h | 12 ++-- 2 files changed, 60 insertions(+), 49 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 039f818a8c0..3be55e9985d 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -490,7 +490,7 @@ void SchemaBuilder::applyDiff(const SchemaDiff & diff) auto db_info = getter.getDatabase(opt.schema_id); if (db_info == nullptr) throw TiFlashException("miss database: " + std::to_string(diff.schema_id), Errors::DDL::StaleSchema); - applyRenameTable(db_info, opt.table_id); + applyRenameTable(db_info, opt.table_id, "RenameTables"); } return; } @@ -542,7 +542,7 @@ void SchemaBuilder::applyDiff(const SchemaDiff & diff) } case SchemaActionType::RenameTable: { - applyRenameTable(db_info, diff.table_id); + applyRenameTable(db_info, diff.table_id, "RenameTable"); break; } case SchemaActionType::AddTablePartition: @@ -580,7 +580,8 @@ void SchemaBuilder::applyDiff(const SchemaDiff & diff) if (old_table_id) { - applyDropTable(db_info, old_table_id); + String action = fmt::format("{}", magic_enum::enum_name(diff.type)); + applyDropTable(db_info, old_table_id, action); } if (new_table_id) @@ -595,11 +596,11 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr auto table_info = getter.getTableInfo(db_info->id, table_id); if (table_info == nullptr) { - throw TiFlashException(fmt::format("miss old table id in TiKV {}", table_id), Errors::DDL::StaleSchema); + throw TiFlashException(fmt::format("miss old table id in TiKV when applyPartitionDiff, table_id={}", table_id), Errors::DDL::StaleSchema); } if (!table_info->isLogicalPartitionTable()) { - throw TiFlashException(fmt::format("new table in TiKV not partition table {}", name_mapper.debugCanonicalName(*db_info, *table_info)), + throw TiFlashException(fmt::format("new table in TiKV not partition table when applyPartitionDiff, {}", name_mapper.debugCanonicalName(*db_info, *table_info)), Errors::DDL::TableTypeNotMatch); } @@ -607,10 +608,10 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr auto storage = tmt_context.getStorages().get(keyspace_id, table_info->id); if (storage == nullptr) { - throw TiFlashException(fmt::format("miss table in TiFlash {}", table_id), Errors::DDL::MissingTable); + throw TiFlashException(fmt::format("miss table in TiFlash when applyPartitionDiff, table_id={}", table_id), Errors::DDL::MissingTable); } - applyPartitionDiff(db_info, table_info, storage, /*drop_part_if_not_exist*/ true); + applyPartitionDiffOnLogicalTable(db_info, table_info, storage, /*drop_part_if_not_exist*/ true); } template @@ -644,7 +645,7 @@ TiDB::DBInfoPtr SchemaBuilder::tryFindDatabaseByPartitionTab } template -void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist) +void SchemaBuilder::applyPartitionDiffOnLogicalTable(const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist) { const auto & orig_table_info = storage->getTableInfo(); if (!orig_table_info.isLogicalPartitionTable()) @@ -693,10 +694,10 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr if (!new_part_id_set.contains(orig_def.id)) { const auto part_table_name = name_mapper.mapTableNameByID(updated_table_info.keyspace_id, orig_def.id); - // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchem` -> `applyPartitionDiff` without `applyExchangeTablePartition` + // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchema` -> `applyPartitionDiffOnLogicalTable` without `applyExchangeTablePartition` // The physical table maybe `EXCHANGE` to another database, try to find the partition from all database auto part_db_info = tryFindDatabaseByPartitionTable(db_info, part_table_name); - applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, "exchange partition"); } } } @@ -707,7 +708,7 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr { auto part_table_info = updated_table_info.producePartitionTableInfo(new_def.id, name_mapper); const auto part_table_name = name_mapper.mapTableName(*part_table_info); - // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchem` -> `applyPartitionDiff` without `applyExchangeTablePartition` + // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchema` -> `applyPartitionDiffOnLogicalTable` without `applyExchangeTablePartition` // The physical table maybe `EXCHANGE` from another database, try to find the partition from all database auto part_db_info = tryFindDatabaseByPartitionTable(db_info, part_table_name); applyCreatePhysicalTable(part_db_info, part_table_info); @@ -722,31 +723,32 @@ void SchemaBuilder::applyPartitionDiff(const TiDB::DBInfoPtr } template -void SchemaBuilder::applyRenameTable(const DBInfoPtr & new_db_info, TableID table_id) +void SchemaBuilder::applyRenameTable(const DBInfoPtr & new_db_info, TableID table_id, std::string_view action) { auto new_table_info = getter.getTableInfo(new_db_info->id, table_id); if (new_table_info == nullptr) { - throw TiFlashException(fmt::format("miss table id in TiKV {}", table_id), Errors::DDL::StaleSchema); + throw TiFlashException(fmt::format("miss table id in TiKV when renameTable, table_id={} action={}", table_id, action), Errors::DDL::StaleSchema); } auto & tmt_context = context.getTMTContext(); auto storage = tmt_context.getStorages().get(keyspace_id, table_id); if (storage == nullptr) { - throw TiFlashException(fmt::format("miss table id in TiFlash {}", table_id), Errors::DDL::MissingTable); + throw TiFlashException(fmt::format("miss table id in TiFlash when renameTable, table_id={} action={}", table_id, action), Errors::DDL::MissingTable); } - applyRenameLogicalTable(new_db_info, new_table_info, storage); + applyRenameLogicalTable(new_db_info, new_table_info, storage, action); } template void SchemaBuilder::applyRenameLogicalTable( const DBInfoPtr & new_db_info, const TableInfoPtr & new_table_info, - const ManageableStoragePtr & storage) + const ManageableStoragePtr & storage, + std::string_view action) { - applyRenamePhysicalTable(new_db_info, *new_table_info, storage); + applyRenamePhysicalTable(new_db_info, *new_table_info, storage, action); if (new_table_info->isLogicalPartitionTable()) { @@ -756,10 +758,16 @@ void SchemaBuilder::applyRenameLogicalTable( auto part_storage = tmt_context.getStorages().get(keyspace_id, part_def.id); if (part_storage == nullptr) { - throw Exception(fmt::format("miss old table id in Flash {}", part_def.id)); + throw Exception( // + ErrorCodes::LOGICAL_ERROR, + "Storage instance is not exist in TiFlash, the partition is not created yet in this TiFlash instance, " + "physical_table_id={} logical_table_id={} action={}", + part_def.id, + new_table_info->id, + action); } auto part_table_info = new_table_info->producePartitionTableInfo(part_def.id, name_mapper); - applyRenamePhysicalTable(new_db_info, *part_table_info, part_storage); + applyRenamePhysicalTable(new_db_info, *part_table_info, part_storage, action); } } } @@ -768,7 +776,8 @@ template void SchemaBuilder::applyRenamePhysicalTable( const DBInfoPtr & new_db_info, const TableInfo & new_table_info, - const ManageableStoragePtr & storage) + const ManageableStoragePtr & storage, + std::string_view action) { const auto old_mapped_db_name = storage->getDatabaseName(); const auto new_mapped_db_name = name_mapper.mapDatabaseName(*new_db_info); @@ -776,7 +785,7 @@ void SchemaBuilder::applyRenamePhysicalTable( const auto new_display_table_name = name_mapper.displayTableName(new_table_info); if (old_mapped_db_name == new_mapped_db_name && old_display_table_name == new_display_table_name) { - LOG_DEBUG(log, "Table {} name identical, not renaming.", name_mapper.debugCanonicalName(*new_db_info, new_table_info)); + LOG_DEBUG(log, "Table {} name identical, not renaming, action={}", name_mapper.debugCanonicalName(*new_db_info, new_table_info), action); return; } @@ -784,11 +793,12 @@ void SchemaBuilder::applyRenamePhysicalTable( GET_METRIC(tiflash_schema_internal_ddl_count, type_rename_column).Increment(); LOG_INFO( log, - "Renaming table {}.{} (display name: {}) to {}.", + "Renaming table {}.{} (display name: {}) to {}, action={}", old_mapped_db_name, old_mapped_tbl_name, old_display_table_name, - name_mapper.debugCanonicalName(*new_db_info, new_table_info)); + name_mapper.debugCanonicalName(*new_db_info, new_table_info), + action); // Note that rename will update table info in table create statement by modifying original table info // with "tidb_display.table" instead of using new_table_info directly, so that other changes @@ -804,11 +814,12 @@ void SchemaBuilder::applyRenamePhysicalTable( LOG_INFO( log, - "Renamed table {}.{} (display name: {}) to {}", + "Renamed table {}.{} (display name: {}) to {}, action={}", old_mapped_db_name, old_mapped_tbl_name, old_display_table_name, - name_mapper.debugCanonicalName(*new_db_info, new_table_info)); + name_mapper.debugCanonicalName(*new_db_info, new_table_info), + action); } template @@ -877,7 +888,7 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema // once it became a partition. // But this method will skip dropping partition id that is not exist in the new table_info, // because the physical table could be changed into a normal table without dropping. - applyPartitionDiff(pt_db_info, table_info, storage, /*drop_part_if_not_exist*/ false); + applyPartitionDiffOnLogicalTable(pt_db_info, table_info, storage, /*drop_part_if_not_exist*/ false); FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_after_step_1_in_exchange_partition); /// step 2 change non partition table to a partition of the partition table @@ -903,7 +914,7 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_before_step_2_rename_in_exchange_partition); if (npt_db_info->id != pt_db_info->id) - applyRenamePhysicalTable(pt_db_info, orig_table_info, storage); + applyRenamePhysicalTable(pt_db_info, orig_table_info, storage, "exchange partition"); FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_after_step_2_in_exchange_partition); /// step 3 change partition of the partition table to non partition table @@ -935,9 +946,9 @@ void SchemaBuilder::applyExchangeTablePartition(const Schema FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_before_step_3_rename_in_exchange_partition); if (npt_db_info->id != pt_db_info->id) - applyRenamePhysicalTable(npt_db_info, orig_table_info, storage); + applyRenamePhysicalTable(npt_db_info, orig_table_info, storage, "exchange partition"); FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_after_step_3_in_exchange_partition); - LOG_INFO(log, "Execute exchange partition done, npt_table_id={} npt_database_id={} pt_table_id={} pt_partition_id={} pt_database_id={}", npt_table_id, npt_database_id, pt_table_id, pt_partition_id, pt_database_id); + LOG_INFO(log, "Execute exchange partition done, npt_table_id={} npt_database_id={} pt_table_id={} pt_partition_id={} pt_database_id={}", npt_table_id, npt_database_id, pt_table_id, pt_partition_id, pt_database_id); } template @@ -1033,11 +1044,11 @@ template void SchemaBuilder::applyDropSchema(const String & db_name) { GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_db).Increment(); - LOG_INFO(log, "Tombstoning database {}", db_name); + LOG_INFO(log, "Tombstone database begin, db_name={}", db_name); auto db = context.tryGetDatabase(db_name); if (db == nullptr) { - LOG_INFO(log, "Database {} does not exists", db_name); + LOG_INFO(log, "Database does not exists, db_name={}", db_name); return; } @@ -1054,7 +1065,7 @@ void SchemaBuilder::applyDropSchema(const String & db_name) auto tombstone = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime); db->alterTombstone(context, tombstone, /*new_db_info*/ nullptr); // keep the old db_info - LOG_INFO(log, "Tombstoned database {}, tombstone={}", db_name, tombstone); + LOG_INFO(log, "Tombstone database end, db_name={} tombstone={}", db_name, tombstone); } template @@ -1276,25 +1287,25 @@ void SchemaBuilder::applyCreateLogicalTable(const TiDB::DBIn } template -void SchemaBuilder::applyDropPhysicalTable(const String & db_name, TableID table_id) +void SchemaBuilder::applyDropPhysicalTable(const String & db_name, TableID table_id, std::string_view action) { auto & tmt_context = context.getTMTContext(); auto storage = tmt_context.getStorages().get(keyspace_id, table_id); if (storage == nullptr) { - LOG_DEBUG(log, "table {} does not exist.", table_id); + LOG_DEBUG(log, "table does not exist, table_id={} action={}", table_id, action); return; } // Skip updating the tombstone_ts if the table is already tombstone. if (auto tombstone_ts = storage->getTombstone(); tombstone_ts != 0) { - LOG_INFO(log, "Tombstone table {}.{} has been done before, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); + LOG_INFO(log, "Tombstone table {}.{} has been done before, action={} tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action, tombstone_ts); return; } GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); - LOG_INFO(log, "Tombstone table {}.{} begin", db_name, name_mapper.debugTableName(storage->getTableInfo())); + LOG_INFO(log, "Tombstone table {}.{} begin, action={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action); const UInt64 tombstone_ts = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime); AlterCommands commands; { @@ -1310,11 +1321,11 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db } auto alter_lock = storage->lockForAlter(getThreadNameAndID()); storage->alterFromTiDB(alter_lock, commands, db_name, storage->getTableInfo(), name_mapper, context); - LOG_INFO(log, "Tombstone table {}.{} end, tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), tombstone_ts); + LOG_INFO(log, "Tombstone table {}.{} end, action={} tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action, tombstone_ts); } template -void SchemaBuilder::applyDropTable(const DBInfoPtr & db_info, TableID table_id) +void SchemaBuilder::applyDropTable(const DBInfoPtr & db_info, TableID table_id, std::string_view action) { auto & tmt_context = context.getTMTContext(); auto * storage = tmt_context.getStorages().get(keyspace_id, table_id).get(); @@ -1328,13 +1339,13 @@ void SchemaBuilder::applyDropTable(const DBInfoPtr & db_info { for (const auto & part_def : table_info.partition.definitions) { - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id, action); } } // Drop logical table at last, only logical table drop will be treated as "complete". // Intermediate failure will hide the logical table drop so that schema syncing when restart will re-drop all (despite some physical tables may have dropped). - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, action); } template @@ -1467,10 +1478,10 @@ void SchemaBuilder::syncAllSchema() if (table->isLogicalPartitionTable()) { /// Apply partition diff if needed. - applyPartitionDiff(db, table, storage, /*drop_part_if_not_exist*/ true); + applyPartitionDiffOnLogicalTable(db, table, storage, /*drop_part_if_not_exist*/ true); } /// Rename if needed. - applyRenameLogicalTable(db, table, storage); + applyRenameLogicalTable(db, table, storage, "SyncAllSchema"); /// Update replica info if needed. applySetTiFlashReplicaOnLogicalTable(db, table, storage); /// Alter if needed. @@ -1491,7 +1502,7 @@ void SchemaBuilder::syncAllSchema() } if (table_set.count(table_info.id) == 0) { - applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id); + applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id, "SyncAllSchema"); LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info)); } } diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index 6e817a264f2..de2fbf5d1ce 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -71,14 +71,14 @@ struct SchemaBuilder void applyCreatePhysicalTable(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info); - void applyDropTable(const TiDB::DBInfoPtr & db_info, TableID table_id); + void applyDropTable(const TiDB::DBInfoPtr & db_info, TableID table_id, std::string_view action); /// Parameter schema_name should be mapped. - void applyDropPhysicalTable(const String & db_name, TableID table_id); + void applyDropPhysicalTable(const String & db_name, TableID table_id, std::string_view action); void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, TableID table_id); - void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist); + void applyPartitionDiffOnLogicalTable(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist); TiDB::DBInfoPtr tryFindDatabaseByPartitionTable(const TiDB::DBInfoPtr & db_info, const String & part_table_name); void applyAlterTable(const TiDB::DBInfoPtr & db_info, TableID table_id); @@ -87,11 +87,11 @@ struct SchemaBuilder void applyAlterPhysicalTable(const TiDB::DBInfoPtr & db_info, const TiDB::TableInfoPtr & table_info, const ManageableStoragePtr & storage); - void applyRenameTable(const TiDB::DBInfoPtr & new_db_info, TiDB::TableID table_id); + void applyRenameTable(const TiDB::DBInfoPtr & new_db_info, TiDB::TableID table_id, std::string_view action); - void applyRenameLogicalTable(const TiDB::DBInfoPtr & new_db_info, const TiDB::TableInfoPtr & new_table_info, const ManageableStoragePtr & storage); + void applyRenameLogicalTable(const TiDB::DBInfoPtr & new_db_info, const TiDB::TableInfoPtr & new_table_info, const ManageableStoragePtr & storage, std::string_view action); - void applyRenamePhysicalTable(const TiDB::DBInfoPtr & new_db_info, const TiDB::TableInfo & new_table_info, const ManageableStoragePtr & storage); + void applyRenamePhysicalTable(const TiDB::DBInfoPtr & new_db_info, const TiDB::TableInfo & new_table_info, const ManageableStoragePtr & storage, std::string_view action); void applyExchangeTablePartition(const SchemaDiff & diff); From 458ad9e128cd7566980361704231650701e842e4 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 15 Jul 2024 10:12:05 +0800 Subject: [PATCH 4/7] Drop table schema diff should update the tombstone --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 22 +++++++++++++--------- dbms/src/TiDB/Schema/SchemaBuilder.h | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 3be55e9985d..c71729acbef 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -697,7 +697,7 @@ void SchemaBuilder::applyPartitionDiffOnLogicalTable(const T // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchema` -> `applyPartitionDiffOnLogicalTable` without `applyExchangeTablePartition` // The physical table maybe `EXCHANGE` to another database, try to find the partition from all database auto part_db_info = tryFindDatabaseByPartitionTable(db_info, part_table_name); - applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, "exchange partition"); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, /*must_must_must_update_tombstone*/ false, "exchange partition"); } } } @@ -1287,7 +1287,7 @@ void SchemaBuilder::applyCreateLogicalTable(const TiDB::DBIn } template -void SchemaBuilder::applyDropPhysicalTable(const String & db_name, TableID table_id, std::string_view action) +void SchemaBuilder::applyDropPhysicalTable(const String & db_name, TableID table_id, bool must_update_tombstone, std::string_view action) { auto & tmt_context = context.getTMTContext(); auto storage = tmt_context.getStorages().get(keyspace_id, table_id); @@ -1297,11 +1297,15 @@ void SchemaBuilder::applyDropPhysicalTable(const String & db return; } - // Skip updating the tombstone_ts if the table is already tombstone. - if (auto tombstone_ts = storage->getTombstone(); tombstone_ts != 0) + if (!must_update_tombstone) { - LOG_INFO(log, "Tombstone table {}.{} has been done before, action={} tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action, tombstone_ts); - return; + // When must_update_tombstone == false, we can try to skip updating + // the tombstone_ts if the table is already tombstone. + if (auto tombstone_ts = storage->getTombstone(); tombstone_ts != 0) + { + LOG_INFO(log, "Tombstone table {}.{} has been done before, action={} tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action, tombstone_ts); + return; + } } GET_METRIC(tiflash_schema_internal_ddl_count, type_drop_table).Increment(); @@ -1339,13 +1343,13 @@ void SchemaBuilder::applyDropTable(const DBInfoPtr & db_info { for (const auto & part_def : table_info.partition.definitions) { - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id, action); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id, /*must_update_tombstone*/ true, action); } } // Drop logical table at last, only logical table drop will be treated as "complete". // Intermediate failure will hide the logical table drop so that schema syncing when restart will re-drop all (despite some physical tables may have dropped). - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, action); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, /*must_update_tombstone*/ true, action); } template @@ -1502,7 +1506,7 @@ void SchemaBuilder::syncAllSchema() } if (table_set.count(table_info.id) == 0) { - applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id, "SyncAllSchema"); + applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id, /*must_update_tombstone*/ false, "SyncAllSchema"); LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info)); } } diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.h b/dbms/src/TiDB/Schema/SchemaBuilder.h index de2fbf5d1ce..b85e7a58d9e 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.h +++ b/dbms/src/TiDB/Schema/SchemaBuilder.h @@ -74,7 +74,7 @@ struct SchemaBuilder void applyDropTable(const TiDB::DBInfoPtr & db_info, TableID table_id, std::string_view action); /// Parameter schema_name should be mapped. - void applyDropPhysicalTable(const String & db_name, TableID table_id, std::string_view action); + void applyDropPhysicalTable(const String & db_name, TableID table_id, bool must_update_tombstone, std::string_view action); void applyPartitionDiff(const TiDB::DBInfoPtr & db_info, TableID table_id); From 4a191a842759abf92d3f8bc1f0e8dd8f6650821d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 15 Jul 2024 14:27:24 +0800 Subject: [PATCH 5/7] Refine logging about drop all schema --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index c71729acbef..d1dcaf6f141 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -1537,6 +1537,7 @@ void SchemaBuilder::dropAllSchema() auto & tmt_context = context.getTMTContext(); + const String action = "DropAllSchema"; /// Drop all tables. auto storage_map = tmt_context.getStorages().getAllStorage(); for (const auto & storage : storage_map) @@ -1546,7 +1547,7 @@ void SchemaBuilder::dropAllSchema() { continue; } - applyDropPhysicalTable(storage.second->getDatabaseName(), table_info.id); + applyDropPhysicalTable(storage.second->getDatabaseName(), table_info.id, /*must_update_tombstone*/ false, action); LOG_DEBUG(log, "Table {}.{} dropped during drop all schemas", storage.second->getDatabaseName(), name_mapper.debugTableName(table_info)); } From 3772af10aab55e9dce2f8df8a25b155461859fc1 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 30 Aug 2024 17:51:16 +0800 Subject: [PATCH 6/7] Add comments --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index d1dcaf6f141..8fb0ef063b1 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -697,7 +697,7 @@ void SchemaBuilder::applyPartitionDiffOnLogicalTable(const T // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchema` -> `applyPartitionDiffOnLogicalTable` without `applyExchangeTablePartition` // The physical table maybe `EXCHANGE` to another database, try to find the partition from all database auto part_db_info = tryFindDatabaseByPartitionTable(db_info, part_table_name); - applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, /*must_must_must_update_tombstone*/ false, "exchange partition"); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, /*must_update_tombstone*/ false, "exchange partition"); } } } @@ -1338,18 +1338,23 @@ void SchemaBuilder::applyDropTable(const DBInfoPtr & db_info LOG_DEBUG(log, "table {} does not exist.", table_id); return; } + + // `applyDropTable` is only called by applying schema diff. So we should update tombstone_ts. + // If a table is dropped at tso_1. Then the table is recorvered, then droppped again at tso_2. + // When tiflash meet the SchemaDiff of "drop table" at tso_2, it should update the tombstone_ts. + const bool update_tombstone_ts = true; const auto & table_info = storage->getTableInfo(); if (table_info.isLogicalPartitionTable()) { for (const auto & part_def : table_info.partition.definitions) { - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id, /*must_update_tombstone*/ true, action); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), part_def.id, update_tombstone_ts, action); } } // Drop logical table at last, only logical table drop will be treated as "complete". // Intermediate failure will hide the logical table drop so that schema syncing when restart will re-drop all (despite some physical tables may have dropped). - applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, /*must_update_tombstone*/ true, action); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, update_tombstone_ts, action); } template @@ -1506,6 +1511,7 @@ void SchemaBuilder::syncAllSchema() } if (table_set.count(table_info.id) == 0) { + // If the table is already tombstone, don't need to update its tombstone_ts. applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id, /*must_update_tombstone*/ false, "SyncAllSchema"); LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info)); } From bea569946245820167197f6187b42cec641f1df4 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Tue, 3 Sep 2024 11:07:01 +0800 Subject: [PATCH 7/7] Align with master --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 8fb0ef063b1..c838a533069 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -687,6 +687,7 @@ void SchemaBuilder::applyPartitionDiffOnLogicalTable(const T updated_table_info.partition = table_info->partition; /// Apply changes to physical tables. + auto reason = fmt::format("ApplyPartitionDiff-logical_table_id={}", orig_table_info.id); if (drop_part_if_not_exist) { for (const auto & orig_def : orig_defs) @@ -697,7 +698,7 @@ void SchemaBuilder::applyPartitionDiffOnLogicalTable(const T // When `tryLoadSchemaDiffs` fails, we may run into `SchemaBuilder::syncAllSchema` -> `applyPartitionDiffOnLogicalTable` without `applyExchangeTablePartition` // The physical table maybe `EXCHANGE` to another database, try to find the partition from all database auto part_db_info = tryFindDatabaseByPartitionTable(db_info, part_table_name); - applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, /*must_update_tombstone*/ false, "exchange partition"); + applyDropPhysicalTable(name_mapper.mapDatabaseName(*part_db_info), orig_def.id, /*must_update_tombstone*/ false, reason); } } }