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.

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: Fix database drop by accident when meet regenerate_schema_map flag #8785

Merged
merged 4 commits into from
Feb 26, 2024

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Feb 23, 2024

What problem does this PR solve?

Issue Number: close #8777

Problem Summary:

Normally, after users executed DDL statements, the TiDB server will generate some "SchemaDiff" to indicate what happens. And TiFlash will follow the "SchemaDiff" to figure out what DDL operations need to be executed.
But when there are special DDL FLASHBACK CLUSTER TO or br restore point (which generate regenerate_schema_map == true), TiFlash will try to list all existing databases and tables in the cluster and enter syncAllSchema to check all existing databases and tables one by one and execute proper DDL on them.

However, in the DDL framwork refactor since v7.2(#7437), there is a mistake in the implementation. In the following codes, only newly created database that does not exist in TiFlash local are put in created_db_set. In line 1383, if the database is not existed in created_db_set, then the database will be marked as tombstone unexpectedly. And after the gc_safepoint advance, the data on TiFlash replica get physically removed unexpectedly. :(

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::syncAllSchema()
{
LOG_INFO(log, "Sync all schemas begin");
/// Create all databases.
std::vector<DBInfoPtr> all_schemas = getter.listDBs();
std::unordered_set<String> created_db_set;
//We can't use too large default_num_threads, otherwise, the lock grabbing time will be too much.
size_t default_num_threads = std::max(4UL, std::thread::hardware_concurrency());
auto sync_all_schema_thread_pool
= ThreadPool(default_num_threads, default_num_threads / 2, default_num_threads * 2);
auto sync_all_schema_wait_group = sync_all_schema_thread_pool.waitGroup();
std::mutex created_db_set_mutex;
for (const auto & db : all_schemas)
{
auto task = [this, db, &created_db_set, &created_db_set_mutex] {
{
std::shared_lock<std::shared_mutex> shared_lock(shared_mutex_for_databases);
if (databases.find(db->id) == databases.end())
{
shared_lock.unlock();
applyCreateSchema(db);
{
std::unique_lock<std::mutex> created_db_set_lock(created_db_set_mutex);
created_db_set.emplace(name_mapper.mapDatabaseName(*db));
}
LOG_INFO(
log,
"Database {} created during sync all schemas, database_id={}",
name_mapper.debugDatabaseName(*db),
db->id);
}
}
std::vector<TableInfoPtr> tables = getter.listTables(db->id);
for (auto & table : tables)
{
LOG_INFO(
log,
"Table {} syncing during sync all schemas, database_id={} table_id={}",
name_mapper.debugCanonicalName(*db, *table),
db->id,
table->id);
/// Ignore view and sequence.
if (table->is_view || table->is_sequence)
{
LOG_INFO(
log,
"Table {} is a view or sequence, ignoring. database_id={} table_id={}",
name_mapper.debugCanonicalName(*db, *table),
db->id,
table->id);
continue;
}
table_id_map.emplaceTableID(table->id, db->id);
LOG_DEBUG(log, "register table to table_id_map, database_id={} table_id={}", db->id, table->id);
applyCreateStorageInstance(db, table);
if (table->isLogicalPartitionTable())
{
for (const auto & part_def : table->partition.definitions)
{
LOG_DEBUG(
log,
"register table to table_id_map for partition table, logical_table_id={} "
"physical_table_id={}",
table->id,
part_def.id);
table_id_map.emplacePartitionTableID(part_def.id, table->id);
}
}
}
};
sync_all_schema_wait_group->schedule(task);
}
sync_all_schema_wait_group->wait();
// TODO:can be removed if we don't save the .sql
/// Drop all unmapped tables.
auto storage_map = context.getTMTContext().getStorages().getAllStorage();
for (auto it = storage_map.begin(); it != storage_map.end(); it++)
{
const auto & table_info = it->second->getTableInfo();
if (table_info.keyspace_id != keyspace_id)
continue;
if (!table_id_map.tableIDInTwoMaps(table_info.id))
{
applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id);
LOG_INFO(
log,
"Table {}.{} dropped during sync all schemas, table_id={}",
it->second->getDatabaseName(),
name_mapper.debugTableName(table_info),
table_info.id);
}
}
/// Drop all unmapped dbs.
const auto & dbs = context.getDatabases();
for (auto it = dbs.begin(); it != dbs.end(); it++)
{
auto db_keyspace_id = SchemaNameMapper::getMappedNameKeyspaceID(it->first);
if (db_keyspace_id != keyspace_id)
{
continue;
}
if (created_db_set.count(it->first) == 0 && !isReservedDatabase(context, it->first))
{
applyDropSchema(it->first);
LOG_INFO(log, "Database {} dropped during sync all schemas", it->first);
}
}
LOG_INFO(log, "Sync all schemas end");
}

Though syncAllSchema will also run right after TiFlash restarts, but at that time, the DatabaseInfoCache is empty, so it do not trigger this bug.

What is changed and how it works?

Add the database name to set no matter the database is already present locally or not

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Fix the issue that TiFlash replica data dropped unexpectedly after PITR restore or `FLASHBACK CLUSTER TO` is executed

@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 23, 2024
@JaySon-Huang JaySon-Huang changed the title [WIP] ddl: Fix database drop by accident when meet regenerate_schema_map flag ddl: Fix database drop by accident when meet regenerate_schema_map flag Feb 23, 2024
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2024
@JaySon-Huang JaySon-Huang self-assigned this Feb 23, 2024
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Feb 23, 2024

Though syncAllSchema will also run after TiFlash restarts, but at that time, the DatabaseInfoCache is empty, so it do not trigger this bug.

The actions that make a SchemaDiff with regenerate_schema_map == true flag

@JaySon-Huang
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@Lloyd-Pottiger Lloyd-Pottiger left a comment

Choose a reason for hiding this comment

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

lgtm

Does this mean when meets diff->regenerate_schema_map = true, all databases in Database info cache will be dropped in previous implement?

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Feb 23, 2024
@JaySon-Huang
Copy link
Contributor Author

JaySon-Huang commented Feb 23, 2024

Does this mean when meets diff->regenerate_schema_map = true, all databases in Database info cache will be dropped in previous implement?

Yes. The v7.1 and oldest versions implement it correctly. The implementation in current master/7.5 branch is a mistake after DDL framework refactor since v7.2.

codes in v7.1:

template <typename Getter, typename NameMapper>
void SchemaBuilder<Getter, NameMapper>::syncAllSchema()
{
LOG_INFO(log, "Syncing all schemas.");
auto & tmt_context = context.getTMTContext();
/// Create all databases.
std::unordered_set<String> db_set;
std::vector<DBInfoPtr> all_schemas = getter.listDBs();
for (const auto & db : all_schemas)
{
db_set.emplace(name_mapper.mapDatabaseName(*db));
if (databases.find(KeyspaceDatabaseID{keyspace_id, db->id}) == databases.end())
{
applyCreateSchema(db);
LOG_INFO(log, "Database {} created during sync all schemas", name_mapper.debugDatabaseName(*db));
}
}
/// Load all tables in each database.
std::unordered_set<TableID> table_set;
for (const auto & db : all_schemas)
{
std::vector<TableInfoPtr> tables = getter.listTables(db->id);
for (auto & table : tables)
{
LOG_INFO(log, "Table {} syncing during sync all schemas", name_mapper.debugCanonicalName(*db, *table));
/// Ignore view and sequence.
if (table->is_view || table->is_sequence)
{
LOG_INFO(log, "Table {} is a view or sequence, ignoring.", name_mapper.debugCanonicalName(*db, *table));
continue;
}
/// Record for further detecting tables to drop.
table_set.emplace(table->id);
if (table->isLogicalPartitionTable())
{
std::for_each(table->partition.definitions.begin(), table->partition.definitions.end(), [&table_set](const auto & def) {
table_set.emplace(def.id);
});
}
auto storage = tmt_context.getStorages().get(keyspace_id, table->id);
if (storage == nullptr)
{
/// Create if not exists.
applyCreateLogicalTable(db, table);
storage = tmt_context.getStorages().get(keyspace_id, table->id);
if (storage == nullptr)
{
/// This is abnormal as the storage shouldn't be null after creation, the underlying table must already be existing for unknown reason.
LOG_WARNING(log,
"Table {} not synced because may have been dropped during sync all schemas",
name_mapper.debugCanonicalName(*db, *table));
continue;
}
}
if (table->isLogicalPartitionTable())
{
/// Apply partition diff if needed.
applyPartitionDiff(db, table, storage, /*drop_part_if_not_exist*/ true);
}
/// Rename if needed.
applyRenameLogicalTable(db, table, storage);
/// Update replica info if needed.
applySetTiFlashReplicaOnLogicalTable(db, table, storage);
/// Alter if needed.
applyAlterLogicalTable(db, table, storage);
LOG_INFO(log, "Table {} synced during sync all schemas", name_mapper.debugCanonicalName(*db, *table));
}
LOG_INFO(log, "Database {} synced during sync all schemas", name_mapper.debugDatabaseName(*db));
}
/// Drop all unmapped tables.
auto storage_map = tmt_context.getStorages().getAllStorage();
for (auto it = storage_map.begin(); it != storage_map.end(); it++)
{
auto table_info = it->second->getTableInfo();
if (table_info.keyspace_id != keyspace_id)
{
continue;
}
if (table_set.count(table_info.id) == 0)
{
applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id);
LOG_INFO(log, "Table {}.{} dropped during sync all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info));
}
}
/// Drop all unmapped dbs.
const auto & dbs = context.getDatabases();
for (auto it = dbs.begin(); it != dbs.end(); it++)
{
auto db_keyspace_id = SchemaNameMapper::getMappedNameKeyspaceID(it->first);
if (db_keyspace_id != keyspace_id)
{
continue;
}
if (db_set.count(it->first) == 0 && !isReservedDatabase(context, it->first))
{
applyDropSchema(it->first);
LOG_INFO(log, "DB {} dropped during sync all schemas", it->first);
}
}
LOG_INFO(log, "Loaded all schemas.");
}

@JaySon-Huang
Copy link
Contributor Author

/run-integration-test

@JaySon-Huang
Copy link
Contributor Author

/run-integration-test

@ti-chi-bot ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Feb 26, 2024
@JaySon-Huang JaySon-Huang removed the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Feb 26, 2024
@ti-chi-bot ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Feb 26, 2024
Copy link
Contributor

ti-chi-bot bot commented Feb 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JinheLin, Lloyd-Pottiger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [JinheLin,Lloyd-Pottiger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 26, 2024
Copy link
Contributor

ti-chi-bot bot commented Feb 26, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-02-23 14:14:35.687536094 +0000 UTC m=+625764.435159198: ☑️ agreed by Lloyd-Pottiger.
  • 2024-02-26 07:30:16.703489267 +0000 UTC m=+860705.451112378: ☑️ agreed by JinheLin.

@ti-chi-bot ti-chi-bot bot merged commit 1ecbbcd into pingcap:master Feb 26, 2024
6 checks passed
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: failed to apply #8785 on top of branch "release-7.5":

failed to git commit: exit status 1

@JaySon-Huang JaySon-Huang deleted the fix_database_drop_by_accident branch February 26, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After upgrade from 5.4 to 7.5, my TiFlash data gone after the TiKV node disconnected
4 participants