From b38d315db7c7532b2d27ee2358db65d3fade5bcd Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger Date: Fri, 17 Jan 2025 10:11:12 +0800 Subject: [PATCH 1/2] ddl: fix query partition table failed after rename column Signed-off-by: Lloyd-Pottiger --- .../Coprocessor/DAGStorageInterpreter.cpp | 90 ++++++++++++++----- .../ddl/partitions/rename_column.test | 29 ++++++ 2 files changed, 95 insertions(+), 24 deletions(-) create mode 100644 tests/fullstack-test2/ddl/partitions/rename_column.test diff --git a/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp b/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp index 19de268f610..6ba5e905d99 100644 --- a/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp +++ b/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp @@ -1389,6 +1389,26 @@ std::unordered_map DAG return {nullptr, {}}; }; + // Check whether the schema of all tables are the same. + // Only check column names is enough since we will check other properties in compareColumns. + auto check_storages_schema_same = [&](const std::vector & table_storages) -> bool { + if (table_storages.size() <= 1) + return true; + auto table_columns = table_storages[0]->getTableInfo().columns; + for (size_t i = 1; i < table_storages.size(); ++i) + { + const auto & columns = table_storages[i]->getTableInfo().columns; + if (columns.size() != table_columns.size()) + return false; + for (size_t j = 0; j < columns.size(); ++j) + { + if (columns[j].name != table_columns[j].name) + return false; + } + } + return true; + }; + auto get_and_lock_storages = [&](bool schema_synced) -> std::tuple, std::vector, std::vector> { std::vector table_storages; @@ -1431,6 +1451,16 @@ std::unordered_map DAG } } + if (!check_storages_schema_same(table_storages)) + { + // Since we can not know which table's schema is newer, we need to sync all tables' schema. + need_sync_table_ids.clear(); + need_sync_table_ids.append_range(table_scan.getPhysicalTableIDs()); + need_sync_table_ids.push_back(logical_table_id); + table_storages.clear(); + table_locks.clear(); + } + if (need_sync_table_ids.empty()) return {table_storages, table_locks, need_sync_table_ids}; // If we need to syncSchemas, we cannot hold the lock of tables. @@ -1448,20 +1478,24 @@ std::unordered_map DAG log, "Table schema sync done, keyspace={} table_id={} cost={} ms", keyspace_id, - logical_table_id, + table_id, schema_sync_cost); }; - /// Try get storage and lock once. - auto [storages, locks, need_sync_table_ids] = get_and_lock_storages(false); - if (need_sync_table_ids.empty()) - { - LOG_DEBUG(log, "OK, no syncing required."); - } - else - /// If first try failed, sync schema and try again. - { - LOG_INFO(log, "not OK, syncing schemas."); + auto sync_schema_for_needed = [&](bool schema_synced) { + auto [storages, locks, need_sync_table_ids] = get_and_lock_storages(schema_synced); + if (need_sync_table_ids.empty()) + { + for (size_t i = 0; i < storages.size(); ++i) + { + auto const table_id = storages[i]->getTableInfo().id; + storages_with_lock[table_id] = {std::move(storages[i]), std::move(locks[i])}; + } + LOG_DEBUG(log, "OK, no syncing required."); + return true; + } + + LOG_DEBUG(log, "not OK, syncing schemas for keyspace={} table_ids={}", keyspace_id, need_sync_table_ids); auto start_time = Clock::now(); for (auto & table_id : need_sync_table_ids) @@ -1472,21 +1506,29 @@ std::unordered_map DAG = std::chrono::duration_cast(Clock::now() - start_time).count(); LOG_INFO(log, "syncing schemas done, time cost = {} ms.", schema_sync_cost); + return false; + }; + // sync schema + bool success = sync_schema_for_needed(false); + // if failed, try again. + success = success || sync_schema_for_needed(true); + // if failed, try again. + // This used to handle the rename partition table column case. + // Example: We have a partition table named `t1` with 2 partitions `p1` and `p2`. + // `t1` and `p1` have the same old schema, but `p2` does not have schema. + // Then we rename `c1` to `c2` in `t1`, and query `t1` with `p1` and `p2`. + // In this case, we need to sync schema for `p2` first. + // Then we can get the schema of `p2` and `t1` are different, and we need to sync schema for `t1`, `p1` and `p2`. + // Last, call `sync_schema_for_needed` again to make sure all tables' schema are synced. + // Q & A: Why not sync all tables' schema at the first time? + // For example, we have another partition table named `t2` with 2 partitions `p3` and `p4`. + // `t2`, `p3` and `p4` have the same schema, but then `p4` is truncated and send a query to `t2`. + // Since storage of `p4` is dropped, and we will try to sync schema for `t2`, `p3` and `p4`. + // But `p4` does not exist in `t2`'s schema, so we will get an exception "Table doesn't exist". + success = success || sync_schema_for_needed(true); + RUNTIME_CHECK_MSG(success, "Failed to sync schema for all tables."); - std::tie(storages, locks, need_sync_table_ids) = get_and_lock_storages(true); - if (need_sync_table_ids.empty()) - { - LOG_DEBUG(log, "OK after syncing."); - } - else - throw TiFlashException("Shouldn't reach here", Errors::Coprocessor::Internal); - } - for (size_t i = 0; i < storages.size(); ++i) - { - auto const table_id = storages[i]->getTableInfo().id; - storages_with_lock[table_id] = {std::move(storages[i]), std::move(locks[i])}; - } return storages_with_lock; } diff --git a/tests/fullstack-test2/ddl/partitions/rename_column.test b/tests/fullstack-test2/ddl/partitions/rename_column.test new file mode 100644 index 00000000000..319225dbd17 --- /dev/null +++ b/tests/fullstack-test2/ddl/partitions/rename_column.test @@ -0,0 +1,29 @@ +# Copyright 2025 PingCAP, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +mysql> drop table if exists test.t + +mysql> create table test.t (`col5def` mediumint DEFAULT '1398811', `colb0ec` datetime DEFAULT '8060-08-20 09:18:05', `colc4d2` year(4) DEFAULT '2123', `colf318` enum('vzd','f1y','wndk','bdw9','qkg','pj','z3','6pj2q','zgm','x5qj','uiyv') DEFAULT 'bdw9', `3f60e6b3` decimal(33,25) DEFAULT '-66552329.3166265', `colf8a2` bigint DEFAULT '600923851820286643') PARTITION BY HASH (`colf8a2`) PARTITIONS 9; +mysql> insert into test.t values (); +mysql> alter table test.t set tiflash replica 1; + +mysql> alter table test.t change 3f60e6b3 3f60e6b2 decimal(33,25) DEFAULT '-66552329.3166265'; +mysql> set tidb_enforce_mpp=1; select * from test.t; ++---------+---------------------+---------+---------+-------------------------------------+--------------------+ +| col5def | colb0ec | colc4d2 | colf318 | 3f60e6b2 | colf8a2 | ++---------+---------------------+---------+---------+-------------------------------------+--------------------+ +| 1398811 | 8060-08-20 09:18:05 | 2123 | bdw9 | -66552329.3166265000000000000000000 | 600923851820286643 | ++---------+---------------------+---------+---------+-------------------------------------+--------------------+ + +mysql> drop table test.t; From 33e50b2f6db7b3c129c79ba5b095004f23d771be Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger Date: Tue, 21 Jan 2025 12:07:57 +0800 Subject: [PATCH 2/2] address comments Signed-off-by: Lloyd-Pottiger --- dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp b/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp index 6ba5e905d99..db8acb4c9ef 100644 --- a/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp +++ b/dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp @@ -1455,7 +1455,10 @@ std::unordered_map DAG { // Since we can not know which table's schema is newer, we need to sync all tables' schema. need_sync_table_ids.clear(); - need_sync_table_ids.append_range(table_scan.getPhysicalTableIDs()); + need_sync_table_ids.insert( + need_sync_table_ids.end(), + table_scan.getPhysicalTableIDs().begin(), + table_scan.getPhysicalTableIDs().end()); need_sync_table_ids.push_back(logical_table_id); table_storages.clear(); table_locks.clear(); @@ -1495,7 +1498,7 @@ std::unordered_map DAG return true; } - LOG_DEBUG(log, "not OK, syncing schemas for keyspace={} table_ids={}", keyspace_id, need_sync_table_ids); + LOG_INFO(log, "not OK, syncing schemas for keyspace={} table_ids={}", keyspace_id, need_sync_table_ids); auto start_time = Clock::now(); for (auto & table_id : need_sync_table_ids)