ddl: fix query partition table failed after rename column (#9789)

close #9787

Sync schema for all partitions if the schema of one of physical tables is different from the logical table.

Signed-off-by: Lloyd-Pottiger <[email protected]>

dbms/src/Flash/Coprocessor/DAGStorageInterpreter.cpp

@@ -1391,6 +1391,26 @@ std::unordered_map<TableID, DAGStorageInterpreter::StorageWithStructureLock> 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<ManageableStoragePtr> & 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<ManageableStoragePtr>, std::vector<TableStructureLockHolder>, std::vector<TableID>> {
         std::vector<ManageableStoragePtr> table_storages;
@@ -1433,6 +1453,16 @@ std::unordered_map<TableID, DAGStorageInterpreter::StorageWithStructureLock> 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.
@@ -1450,20 +1480,24 @@ std::unordered_map<TableID, DAGStorageInterpreter::StorageWithStructureLock> 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)
@@ -1474,21 +1508,29 @@ std::unordered_map<TableID, DAGStorageInterpreter::StorageWithStructureLock> DAG
             = std::chrono::duration_cast<std::chrono::milliseconds>(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;
 }
 

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;