From 04d99ff73332786e38df560702e8e52a0751d31d Mon Sep 17 00:00:00 2001 From: hongyunyan <649330952@qq.com> Date: Mon, 13 Jun 2022 18:11:14 +0800 Subject: [PATCH 1/4] add comments --- dbms/src/TiDB/Schema/SchemaGetter.h | 1 + 1 file changed, 1 insertion(+) diff --git a/dbms/src/TiDB/Schema/SchemaGetter.h b/dbms/src/TiDB/Schema/SchemaGetter.h index cfa5e1c6335..02d2f7a7c88 100644 --- a/dbms/src/TiDB/Schema/SchemaGetter.h +++ b/dbms/src/TiDB/Schema/SchemaGetter.h @@ -28,6 +28,7 @@ namespace DB { +// The enum results are completely the same as the DDL Action listed in the "parser/model/ddl.go" of TiDB codebase, which must be keeping in sync. enum class SchemaActionType : Int8 { None = 0, From 0b5b6197dd69e3a96489636563a16fef808552f7 Mon Sep 17 00:00:00 2001 From: hongyunyan <649330952@qq.com> Date: Tue, 14 Jun 2022 12:11:52 +0800 Subject: [PATCH 2/4] add comment for SchemaBuilder --- 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 99e540e6c95..4c7fba1f362 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -370,7 +370,8 @@ void SchemaBuilder::applyAlterPhysicalTable(DBInfoPtr db_inf const auto & schema_change = schema_changes[i]; /// Update column infos by applying schema change in this step. schema_change.second(orig_table_info); - /// Update schema version aggressively for the sake of correctness. + /// Update schema version aggressively for the sake of correctness（for read part). + /// In read action, we will use table_info.schema_version(storage_version) and TiDBSchemaSyncer.cur_version(global_version) to compare with query_version, to decide whether we can read under this query_version, or we need to make the schema newer. In our comparison logic, we must ensure storage_version <= gloabl_version. Thus, when the table need apply multi diffs here, we must update schema version aggressively to ensure storage_version <= gloabl_version. The more detail info you can refer the comments in DAGStorageInterpreter::getAndLockStorages. orig_table_info.schema_version = target_version; auto alter_lock = storage->lockForAlter(getThreadName()); storage->alterFromTiDB( From accbd8b6731a64a0a175a1a11b2d464b82906d09 Mon Sep 17 00:00:00 2001 From: hongyunyan <649330952@qq.com> Date: Tue, 14 Jun 2022 14:13:09 +0800 Subject: [PATCH 3/4] Update dbms/src/TiDB/Schema/SchemaBuilder.cpp Co-authored-by: lidezhu <47731263+lidezhu@users.noreply.github.com> --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 4c7fba1f362..8e7b92c6a4f 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -371,7 +371,14 @@ void SchemaBuilder::applyAlterPhysicalTable(DBInfoPtr db_inf /// Update column infos by applying schema change in this step. schema_change.second(orig_table_info); /// Update schema version aggressively for the sake of correctness（for read part). - /// In read action, we will use table_info.schema_version(storage_version) and TiDBSchemaSyncer.cur_version(global_version) to compare with query_version, to decide whether we can read under this query_version, or we need to make the schema newer. In our comparison logic, we must ensure storage_version <= gloabl_version. Thus, when the table need apply multi diffs here, we must update schema version aggressively to ensure storage_version <= gloabl_version. The more detail info you can refer the comments in DAGStorageInterpreter::getAndLockStorages. + /// In read action, we will use table_info.schema_version(storage_version) and TiDBSchemaSyncer.cur_version(global_version) to compare with query_version, to decide whether we can read under this query_version, or we need to make the schema newer. + /// In our comparison logic, we only serve the query when the query schema version meet the criterion: storage_version <= query_version <= global_version(The more detail info you can refer the comments in DAGStorageInterpreter::getAndLockStorages.) + /// And when apply multi diffs here, we only update global_version when all diffs have been applied. + /// So the global_version may be less than the actual "global_version" of the local schema in the process of applying schema changes. + /// And if we don't update the storage_version ahead of time, we may meet the following case when apply multiple diffs: storage_version <= global_version < actual "global_version". + /// If we receive a query with the same version as global_version, we can have the following scenario: storage_version <= global_version == query_version < actual "global_version". + /// And because storage_version <= global_version == query_version meet the criterion of serving the query, the query will be served. But query_version < actual "global_version" indicates that we use a newer schema to server an older query which may cause some inconsistency issue. + /// So we update storage_version aggressively to prevent the above scenario happens. orig_table_info.schema_version = target_version; auto alter_lock = storage->lockForAlter(getThreadName()); storage->alterFromTiDB( From 3f3b1011e57af02d5ffa3970410ac4818131f597 Mon Sep 17 00:00:00 2001 From: hongyunyan <649330952@qq.com> Date: Tue, 14 Jun 2022 14:25:26 +0800 Subject: [PATCH 4/4] fix format --- dbms/src/TiDB/Schema/SchemaBuilder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/TiDB/Schema/SchemaBuilder.cpp b/dbms/src/TiDB/Schema/SchemaBuilder.cpp index 5d97539316b..ae78923fc61 100644 --- a/dbms/src/TiDB/Schema/SchemaBuilder.cpp +++ b/dbms/src/TiDB/Schema/SchemaBuilder.cpp @@ -371,7 +371,7 @@ void SchemaBuilder::applyAlterPhysicalTable(DBInfoPtr db_inf /// Update column infos by applying schema change in this step. schema_change.second(orig_table_info); /// Update schema version aggressively for the sake of correctness（for read part). - /// In read action, we will use table_info.schema_version(storage_version) and TiDBSchemaSyncer.cur_version(global_version) to compare with query_version, to decide whether we can read under this query_version, or we need to make the schema newer. + /// In read action, we will use table_info.schema_version(storage_version) and TiDBSchemaSyncer.cur_version(global_version) to compare with query_version, to decide whether we can read under this query_version, or we need to make the schema newer. /// In our comparison logic, we only serve the query when the query schema version meet the criterion: storage_version <= query_version <= global_version(The more detail info you can refer the comments in DAGStorageInterpreter::getAndLockStorages.) /// And when apply multi diffs here, we only update global_version when all diffs have been applied. /// So the global_version may be less than the actual "global_version" of the local schema in the process of applying schema changes.