From cffc61e6ce008286d6ec5db2c6eb30c29bf065ec Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com> Date: Tue, 23 May 2023 18:45:38 +0800 Subject: [PATCH] fix keep order in table scan is lost when remote read of PartitionTableScan(release-7.1) (#7526) close pingcap/tiflash#7519, close pingcap/tiflash#7527 --- dbms/src/Flash/Coprocessor/TiDBTableScan.cpp | 5 +- .../Storages/DeltaMerge/DeltaMergeStore.cpp | 17 ++-- tests/fullstack-test/issues/issue_7519.test | 88 +++++++++++++++++++ 3 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 tests/fullstack-test/issues/issue_7519.test diff --git a/dbms/src/Flash/Coprocessor/TiDBTableScan.cpp b/dbms/src/Flash/Coprocessor/TiDBTableScan.cpp index a5aa5ded6bc..92a35fdfbc3 100644 --- a/dbms/src/Flash/Coprocessor/TiDBTableScan.cpp +++ b/dbms/src/Flash/Coprocessor/TiDBTableScan.cpp @@ -29,8 +29,10 @@ TiDBTableScan::TiDBTableScan( // Only No-partition table need keep order when tablescan executor required keep order. // If keep_order is not set, keep order for safety. , keep_order(!is_partition_table_scan && (table_scan->tbl_scan().keep_order() || !table_scan->tbl_scan().has_keep_order())) - , is_fast_scan(table_scan->tbl_scan().is_fast_scan()) + , is_fast_scan(is_partition_table_scan ? table_scan->partition_table_scan().is_fast_scan() : table_scan->tbl_scan().is_fast_scan()) { + RUNTIME_CHECK_MSG(!keep_order || pushed_down_filters.empty(), "Bad TiDB table scan executor: push down filter is not empty when keep order is true"); + if (is_partition_table_scan) { if (table_scan->partition_table_scan().has_table_id()) @@ -79,6 +81,7 @@ void TiDBTableScan::constructTableScanForRemoteRead(tipb::TableScan * tipb_table for (auto id : partition_table_scan.primary_prefix_column_ids()) tipb_table_scan->add_primary_prefix_column_ids(id); tipb_table_scan->set_is_fast_scan(partition_table_scan.is_fast_scan()); + tipb_table_scan->set_keep_order(false); } else { diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 5e8b821f4f8..eb27136111c 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -1020,7 +1020,10 @@ static ReadMode getReadModeImpl(const Context & db_context, bool is_fast_scan, b ReadMode DeltaMergeStore::getReadMode(const Context & db_context, bool is_fast_scan, bool keep_order, const PushDownFilterPtr & filter) { auto read_mode = getReadModeImpl(db_context, is_fast_scan, keep_order); - RUNTIME_CHECK_MSG(!filter || !filter->before_where || read_mode == ReadMode::Bitmap, "Push down filters needs bitmap"); + RUNTIME_CHECK_MSG(!filter || !filter->before_where || read_mode == ReadMode::Bitmap, + "Push down filters needs bitmap, push down filters is empty: {}, read mode: {}", + filter == nullptr || filter->before_where == nullptr, + magic_enum::enum_name(read_mode)); return read_mode; } @@ -1050,11 +1053,13 @@ BlockInputStreams DeltaMergeStore::read(const Context & db_context, SegmentReadTasks tasks = getReadTasksByRanges(*dm_context, sorted_ranges, num_streams, read_segments, /*try_split_task =*/!enable_read_thread); auto log_tracing_id = getLogTracingId(*dm_context); auto tracing_logger = log->getChild(log_tracing_id); - LOG_DEBUG(tracing_logger, - "Read create segment snapshot done, keep_order={} dt_enable_read_thread={} enable_read_thread={}", - keep_order, - db_context.getSettingsRef().dt_enable_read_thread, - enable_read_thread); + LOG_INFO(tracing_logger, + "Read create segment snapshot done, keep_order={} dt_enable_read_thread={} enable_read_thread={} is_fast_scan={} is_push_down_filter_empty={}", + keep_order, + db_context.getSettingsRef().dt_enable_read_thread, + enable_read_thread, + is_fast_scan, + filter == nullptr || filter->before_where == nullptr); auto after_segment_read = [&](const DMContextPtr & dm_context_, const SegmentPtr & segment_) { // TODO: Update the tracing_id before checkSegmentUpdate? diff --git a/tests/fullstack-test/issues/issue_7519.test b/tests/fullstack-test/issues/issue_7519.test new file mode 100644 index 00000000000..5366180300c --- /dev/null +++ b/tests/fullstack-test/issues/issue_7519.test @@ -0,0 +1,88 @@ +# Copyright 2023 PingCAP, Ltd. +# +# 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. + +# Preparation. +=> DBGInvoke __init_fail_point() + +mysql> drop table if exists test.t +mysql> create table test.t (x int, a varchar(50), y int, t time) partition by range (x) (partition p0 values less than (5), partition p1 values less than (10)); + +mysql> insert into test.t values (1, 'a', 1, '700:11:11.1234'), (2, 'b', 2, '711:12:12.1234'); +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t select * from test.t; +mysql> insert into test.t values (8, 'c', 8, '500:21:21.1234'); + +mysql> alter table test.t set tiflash replica 1; +func> wait_table test t +mysql> analyze table test.t; + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select count(*) from test.t; ++----------+ +| count(*) | ++----------+ +| 16385 | ++----------+ + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select * from test.t where x >= 5 and x < 10; ++------+------+------+-----------+ +| x | a | y | t | ++------+------+------+-----------+ +| 8 | c | 8 | 500:21:21 | ++------+------+------+-----------+ + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select x, a, y, hour(t) from test.t where x >= 5 and x < 10; ++------+------+------+---------+ +| x | a | y | hour(t) | ++------+------+------+---------+ +| 8 | c | 8 | 500 | ++------+------+------+---------+ + +=> DBGInvoke __enable_fail_point(force_remote_read_for_batch_cop) + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select count(*) from test.t; ++----------+ +| count(*) | ++----------+ +| 16385 | ++----------+ + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select * from test.t where x >= 5 and x < 10; ++------+------+------+-----------+ +| x | a | y | t | ++------+------+------+-----------+ +| 8 | c | 8 | 500:21:21 | ++------+------+------+-----------+ + +mysql> set tidb_partition_prune_mode=dynamic; set tidb_enforce_mpp=1; select x, a, y, hour(t) from test.t where x >= 5 and x < 10; ++------+------+------+---------+ +| x | a | y | hour(t) | ++------+------+------+---------+ +| 8 | c | 8 | 500 | ++------+------+------+---------+ + +=> DBGInvoke __disable_fail_point(force_remote_read_for_batch_cop) + +# Clean up. +mysql> drop table if exists test.t;