Fix sync schema after schema version key be removed #7072

yongman · 2023-03-14T08:18:34Z

What problem does this PR solve?

Issue Number: close #7071

Problem Summary:

What is changed and how it works?

If schema version key for keyspace not exists, load all schema and tombstone tables and databases in this keyspace.

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

Release note

None

Signed-off-by: yongman <[email protected]>

ti-chi-bot · 2023-03-14T08:18:35Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JaySon-Huang
Lloyd-Pottiger

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

yongman · 2023-03-14T08:20:35Z

/cc @JaySon-Huang @CalvinNeo

yongman · 2023-03-14T08:29:39Z

/cc @iosmanthus

iosmanthus · 2023-03-14T08:41:30Z

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

+        if (version == -1 && cur_version == version)
+        {
+            // Tables and databases are already tombstoned and waiting for GC.
+            return false;
+        }


I think it's too implicit, the reader does not know the table's dropped in builder.syncAllSchemas. How about constructing a builder here and calling applyDropSchema for each table/database explicitly? /cc @JaySon-Huang

How about constructing a builder here and calling applyDropSchema for each table/database explicitly?

Is there a chance that SchemaGetter::getVersion get -1, but the tables and databases metadata on TiKV does not get fully cleaned yet?

I think it's too implicit

I agree, now it is kind of complicated, we can add a shortcut logic when version < 0 to skip some codes and make the flow more clear.

Is there a chance that SchemaGetter::getVersion get -1, but the tables and databases metadata on TiKV does not get fully cleaned yet?

I think partial deletion of an entire keyspace is illegal.

how do the Regions of a "deleted" keyspace get removed from TiKV? Are they removed one by one?

@JaySon-Huang No, the keyspace range will be delete in two UnsafeDestroyRange requests, one for transaction kv space, the other for raw kv space.

I agree that it's better make the logic more direct, the logic now is to treat the version=-1 as same as version>=0 to sync schema, but return ASAP in tryLoadSchemaDiffs if version=-1.
Emm, is it necessary to implement a branch to syncAllSchemas and maintain the metrics ? 🤦 /cc @JaySon-Huang @iosmanthus

Considering of possibility inconsistency of getVersion and listDBs after UnsafeDestroyRange, we do not need to listDB or listTables if the schema version key not exists. It‘s neccessary to run shortcut logic in this case. We can add a new method SchemaBuilder::dropAllSchema() to resolve this. /cc @JaySon-Huang @iosmanthus

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

Signed-off-by: JaySon-Huang <[email protected]>

Co-authored-by: JaySon <[email protected]>

Signed-off-by: yongman <[email protected]>

yongman · 2023-03-15T03:08:38Z

@JaySon-Huang @iosmanthus PTAL

Signed-off-by: yongman <[email protected]>

dbms/src/TiDB/Schema/SchemaBuilder.cpp

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

Signed-off-by: yongman <[email protected]>

JaySon-Huang

The rest LGTM

dbms/src/TiDB/Schema/SchemaGetter.h

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h

Signed-off-by: yongman <[email protected]>

yongman · 2023-03-16T01:00:52Z

/run-all-tests

Lloyd-Pottiger · 2023-03-16T02:32:52Z

dbms/src/TiDB/Schema/SchemaBuilder.cpp

+    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;
+        }
+        applyDropPhysicalTable(it->second->getDatabaseName(), table_info.id);
+        LOG_DEBUG(log, "Table {}.{} dropped during drop all schemas", it->second->getDatabaseName(), name_mapper.debugTableName(table_info));
+    }


better to use Range-based for loop

@Lloyd-Pottiger This code block is copied from syncAllSchemas, we should keep the code style same?

no, improve it, I think.

Signed-off-by: yongman <[email protected]>

Lloyd-Pottiger · 2023-03-16T03:09:52Z

/merge

ti-chi-bot · 2023-03-16T03:09:53Z

@Lloyd-Pottiger: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-16T03:09:55Z

This pull request has been accepted and is ready to merge.

Commit hash: 80a2e5b

ti-chi-bot · 2023-03-16T03:10:08Z

@yongman: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Fix sync schema after schema version key be removed

579c05e

Signed-off-by: yongman <[email protected]>

ti-chi-bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 14, 2023

ti-chi-bot requested review from CalvinNeo and JaySon-Huang March 14, 2023 08:20

ti-chi-bot requested a review from iosmanthus March 14, 2023 08:29

iosmanthus reviewed Mar 14, 2023

View reviewed changes

ti-chi-bot removed the do-not-merge/needs-triage-completed label Mar 14, 2023

Merge branch 'master' into fix-schema-sync

ce34050

JaySon-Huang reviewed Mar 14, 2023

View reviewed changes

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Show resolved Hide resolved

JaySon-Huang and others added 3 commits March 14, 2023 20:07

Fix log

c88bdd6

Signed-off-by: JaySon-Huang <[email protected]>

Add comments

478ee45

Co-authored-by: JaySon <[email protected]>

Use shortcut logic for schema verison key not exists

469958b

Signed-off-by: yongman <[email protected]>

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 15, 2023

Merge branch 'master' into fix-schema-sync

0d520f1

Fix typo

858992a

Signed-off-by: yongman <[email protected]>

JaySon-Huang reviewed Mar 15, 2023

View reviewed changes

dbms/src/TiDB/Schema/SchemaBuilder.cpp Outdated Show resolved Hide resolved

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Outdated Show resolved Hide resolved

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Outdated Show resolved Hide resolved

JaySon-Huang reviewed Mar 15, 2023

View reviewed changes

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Show resolved Hide resolved

yongman added 3 commits March 15, 2023 13:23

Fix comments and suggestions

9fc445f

Signed-off-by: yongman <[email protected]>

Merge branch 'master' into fix-schema-sync

311defb

Merge branch 'master' into fix-schema-sync

fb5fe17

JaySon-Huang approved these changes Mar 15, 2023

View reviewed changes

dbms/src/TiDB/Schema/SchemaGetter.h Outdated Show resolved Hide resolved

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Outdated Show resolved Hide resolved

dbms/src/TiDB/Schema/TiDBSchemaSyncer.h Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 15, 2023

Fix suggestions

39b05e5

Signed-off-by: yongman <[email protected]>

Merge branch 'master' into fix-schema-sync

ae228c4

Lloyd-Pottiger reviewed Mar 16, 2023

View reviewed changes

Use range-based loop

80a2e5b

Signed-off-by: yongman <[email protected]>

Lloyd-Pottiger approved these changes Mar 16, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 16, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 16, 2023

Merge branch 'master' into fix-schema-sync

465b132

ti-chi-bot merged commit 86050b5 into pingcap:master Mar 16, 2023

JaySon-Huang mentioned this pull request Mar 23, 2023

database "system" is dropped when api_version=2 #7147

Open

ystaticy mentioned this pull request Dec 6, 2023

Introduce Keyspace pingcap/tidb#40425

Open

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sync schema after schema version key be removed #7072

Fix sync schema after schema version key be removed #7072

yongman commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023 •

edited

Loading

yongman commented Mar 14, 2023

yongman commented Mar 14, 2023

iosmanthus Mar 14, 2023 •

edited

Loading

JaySon-Huang Mar 14, 2023 •

edited

Loading

JaySon-Huang Mar 14, 2023

yongman Mar 14, 2023

JaySon-Huang Mar 14, 2023

yongman Mar 14, 2023 •

edited

Loading

yongman Mar 14, 2023 •

edited

Loading

yongman Mar 15, 2023

JaySon-Huang Mar 15, 2023

yongman commented Mar 15, 2023

JaySon-Huang left a comment

yongman commented Mar 16, 2023

Lloyd-Pottiger Mar 16, 2023

yongman Mar 16, 2023 •

edited

Loading

Lloyd-Pottiger Mar 16, 2023

yongman Mar 16, 2023

Lloyd-Pottiger commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

Fix sync schema after schema version key be removed #7072

Fix sync schema after schema version key be removed #7072

Conversation

yongman commented Mar 14, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 14, 2023 • edited Loading

yongman commented Mar 14, 2023

yongman commented Mar 14, 2023

iosmanthus Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

JaySon-Huang Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongman Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

yongman Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongman commented Mar 15, 2023

JaySon-Huang left a comment

Choose a reason for hiding this comment

yongman commented Mar 16, 2023

Choose a reason for hiding this comment

yongman Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lloyd-Pottiger commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 14, 2023 •

edited

Loading

iosmanthus Mar 14, 2023 •

edited

Loading

JaySon-Huang Mar 14, 2023 •

edited

Loading

yongman Mar 14, 2023 •

edited

Loading

yongman Mar 14, 2023 •

edited

Loading

yongman Mar 16, 2023 •

edited

Loading