Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

Merged
merged 9 commits into from
Sep 3, 2024
Merged

ddl: Skip updating the tombstone_ts if the table is already tombstone (release-6.5) #9230

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
17d0130
Add ut
JaySon-Huang Jul 15, 2024
6d21ee1
Skip updating the tombstone_ts if the table is already tombstone
JaySon-Huang Jul 12, 2024
2ab5131
Refine logging
JaySon-Huang Jul 14, 2024
6ce0236
Drop table schema diff should update the tombstone
JaySon-Huang Jul 15, 2024
5ba24d1
Merge branch 'release-6.5' into tombstone_repeat_65
JaySon-Huang Aug 28, 2024
759cbfd
Resolve conflict
JaySon-Huang Aug 30, 2024
5212e81
Add comments
JaySon-Huang Aug 30, 2024
2f6a6f2
Merge branch 'release-6.5' into tombstone_repeat_65
JaySon-Huang Sep 2, 2024
5b9d758
Align with master
JaySon-Huang Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 71 additions & 43 deletions dbms/src/TiDB/Schema/SchemaBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ void SchemaBuilder<Getter, NameMapper>::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;
}
Expand Down Expand Up @@ -536,7 +536,7 @@ void SchemaBuilder<Getter, NameMapper>::applyDiff(const SchemaDiff & diff)
}
case SchemaActionType::RenameTable:
{
applyRenameTable(db_info, diff.table_id);
applyRenameTable(db_info, diff.table_id, "RenameTable");
break;
}
case SchemaActionType::AddTablePartition:
Expand Down Expand Up @@ -573,7 +573,8 @@ void SchemaBuilder<Getter, NameMapper>::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)
Expand All @@ -588,22 +589,22 @@ void SchemaBuilder<Getter, NameMapper>::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);
}

auto & tmt_context = context.getTMTContext();
auto storage = tmt_context.getStorages().get(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 <typename Getter, typename NameMapper>
Expand Down Expand Up @@ -637,7 +638,7 @@ TiDB::DBInfoPtr SchemaBuilder<Getter, NameMapper>::tryFindDatabaseByPartitionTab
}

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::applyPartitionDiff(const TiDB::DBInfoPtr & db_info, const TableInfoPtr & table_info, const ManageableStoragePtr & storage, bool drop_part_if_not_exist)
void SchemaBuilder<Getter, NameMapper>::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())
Expand Down Expand Up @@ -686,10 +687,10 @@ void SchemaBuilder<Getter, NameMapper>::applyPartitionDiff(const TiDB::DBInfoPtr
if (new_part_id_set.count(orig_def.id) == 0)
{
const auto part_table_name = name_mapper.mapTableNameByID(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, /*must_update_tombstone*/ false, "exchange partition");
}
}
}
Expand All @@ -700,7 +701,7 @@ void SchemaBuilder<Getter, NameMapper>::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);
Expand All @@ -715,31 +716,32 @@ void SchemaBuilder<Getter, NameMapper>::applyPartitionDiff(const TiDB::DBInfoPtr
}

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::applyRenameTable(const DBInfoPtr & new_db_info, TableID table_id)
void SchemaBuilder<Getter, NameMapper>::applyRenameTable(const DBInfoPtr & new_db_info, TableID table_id, std::string_view action)
{
Comment on lines +720 to 721
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use enum instead of string_view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I think string_view is better than enum. Because something I attach more info than just a enum name. For example, attaching the table_id

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 682 to 683 in 5b9d758

/// Apply changes to physical tables.
auto reason = fmt::format("ApplyPartitionDiff-logical_table_id={}", orig_table_info.id);

tiflash/dbms/src/TiDB/Schema/SchemaBuilder.cpp

Lines 1241 to 1245 in 5b9d758

// If "CREATE DATABASE" is executed in TiFlash after user has executed "DROP DATABASE"
// in TiDB, then TiFlash may not create the IDatabase instance. Make sure we can access
// to the IDatabase when creating IStorage.
const auto database_mapped_name = name_mapper.mapDatabaseName(*db_info);
ensureLocalDatabaseExist(db_info->id, database_mapped_name, fmt::format("CreatePhysicalTable-table_id={}", table_info->id));

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(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 <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::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())
{
Expand All @@ -749,10 +751,16 @@ void SchemaBuilder<Getter, NameMapper>::applyRenameLogicalTable(
auto part_storage = tmt_context.getStorages().get(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);
}
}
}
Expand All @@ -761,32 +769,33 @@ template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::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);
const auto old_display_table_name = name_mapper.displayTableName(storage->getTableInfo());
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;
}

// There could be a chance that the target database has been dropped in TiKV before
// TiFlash accepts the "create database" schema diff. We need to ensure the local
// database exist before executing renaming.
const auto action = fmt::format("applyRenamePhysicalTable-table_id={}", new_table_info.id);
ensureLocalDatabaseExist(new_db_info->id, new_mapped_db_name, action);
const auto old_mapped_tbl_name = storage->getTableName();
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
Expand All @@ -802,11 +811,12 @@ void SchemaBuilder<Getter, NameMapper>::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 <typename Getter, typename NameMapper>
Expand Down Expand Up @@ -876,7 +886,7 @@ void SchemaBuilder<Getter, NameMapper>::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
Expand All @@ -902,7 +912,7 @@ void SchemaBuilder<Getter, NameMapper>::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
Expand Down Expand Up @@ -934,9 +944,9 @@ void SchemaBuilder<Getter, NameMapper>::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 <typename Getter, typename NameMapper>
Expand Down Expand Up @@ -1031,11 +1041,11 @@ template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::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;
}

Expand All @@ -1052,7 +1062,7 @@ void SchemaBuilder<Getter, NameMapper>::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 <typename Getter, typename NameMapper>
Expand Down Expand Up @@ -1308,17 +1318,29 @@ void SchemaBuilder<Getter, NameMapper>::applyCreateLogicalTable(const TiDB::DBIn
}

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::applyDropPhysicalTable(const String & db_name, TableID table_id)
void SchemaBuilder<Getter, NameMapper>::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(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;
}

if (!must_update_tombstone)
{
// 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();
LOG_INFO(log, "Tombstoning table {}.{}", db_name, name_mapper.debugTableName(storage->getTableInfo()));
LOG_INFO(log, "Tombstone table {}.{} begin, action={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action);
const auto tombstone_ts = PDClientHelper::getTSO(tmt_context.getPDClient(), PDClientHelper::get_tso_maxtime);
AlterCommands commands;
{
Expand All @@ -1334,11 +1356,11 @@ void SchemaBuilder<Getter, NameMapper>::applyDropPhysicalTable(const String & db
}
auto alter_lock = storage->lockForAlter(getThreadName());
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, action={} tombstone={}", db_name, name_mapper.debugTableName(storage->getTableInfo()), action, tombstone_ts);
}

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::applyDropTable(const DBInfoPtr & db_info, TableID table_id)
void SchemaBuilder<Getter, NameMapper>::applyDropTable(const DBInfoPtr & db_info, TableID table_id, std::string_view action)
{
auto & tmt_context = context.getTMTContext();
auto * storage = tmt_context.getStorages().get(table_id).get();
Expand All @@ -1347,18 +1369,23 @@ void SchemaBuilder<Getter, NameMapper>::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);
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);
applyDropPhysicalTable(name_mapper.mapDatabaseName(*db_info), table_info.id, update_tombstone_ts, action);
}

template <typename Getter, typename NameMapper>
Expand Down Expand Up @@ -1491,10 +1518,10 @@ void SchemaBuilder<Getter, NameMapper>::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.
Expand All @@ -1510,7 +1537,8 @@ void SchemaBuilder<Getter, NameMapper>::syncAllSchema()
{
if (table_set.count(it->first) == 0)
{
applyDropPhysicalTable(it->second->getDatabaseName(), it->first);
// If the table is already tombstone, don't need to update its tombstone_ts.
applyDropPhysicalTable(it->second->getDatabaseName(), it->first, /*must_update_tombstone*/ false, "SyncAllSchema");
LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(it->second->getTableInfo()));
}
}
Expand Down
Loading