Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unioned query with var_pop over window may lead to incorrect result #52985

Closed
r33s3n6 opened this issue Apr 29, 2024 · 27 comments · Fixed by #55219 or #55333
Closed

unioned query with var_pop over window may lead to incorrect result #52985

r33s3n6 opened this issue Apr 29, 2024 · 27 comments · Fixed by #55219 or #55333
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. fuzz/randomtest impact/wrong-result severity/major sig/planner SIG: Planner type/bug The issue is confirmed as a bug.

Comments

@r33s3n6
Copy link

r33s3n6 commented Apr 29, 2024

1. Minimal reproduce step (Required)

Firstly, execute init.sql to create the table. Then executing error.sql yields unexpected results. Note that reproducing these results might not be entirely stable. Typically, it can be completed within three attempts. You can try executing error.sql multiple times or execute init.sql again to rebuild the table.

init.sql.txt
error.sql.txt

2. What did you expect to see? (Required)

The SQL statement consists of a UNION of two queries. The first query has four rows with the first column being respectively oyz4u, wcnmm, z36ma, and tl. The first column of the second query is a constant string '1'.

3. What did you see instead (Required)

In the multi-node version, some results other than '1' are outputted, while the single-node version behaves normally.

output_re_main2.log
output_re_single2.log

4. What is your TiDB version? (Required)

Release Version: v8.0.0
Edition: Community
Git Commit Hash: 8ba1fa452b1ccdbfb85879ea94b9254aabba2916
Git Branch: HEAD
UTC Build Time: 2024-03-28 14:22:15
GoVersion: go1.21.4
Race Enabled: false
Check Table Before Drop: false
Store: tikv

topology:

distributed.yaml:

global:
  user: "tidb"
  ssh_port: 22
  deploy_dir: "/tidb-deploy"
  data_dir: "/tidb-data"

pd_servers:
  - host: 10.0.2.31

tidb_servers:
  - host: 10.0.2.21

tikv_servers:
  - host: 10.0.2.11
  - host: 10.0.2.12
  - host: 10.0.2.13

monitoring_servers:
  - host: 10.0.2.8

grafana_servers:
  - host: 10.0.2.8

alertmanager_servers:
  - host: 10.0.2.8

tiflash_servers:
  - host: 10.0.2.32

single.yaml

global:
  user: "tidb"
  ssh_port: 22
  deploy_dir: "/tidb-deploy"
  data_dir: "/tidb-data"

pd_servers:
  - host: 10.0.2.73

tidb_servers:
  - host: 10.0.2.72

tikv_servers:
  - host: 10.0.2.71

tiflash_servers:
  - host: 10.0.2.74

about us

We are the BASS team from the School of Cyber Science and Technology at Beihang University. Our main focus is on system software security, operating systems, and program analysis research, as well as the development of automated program testing frameworks for detecting software defects. Using our self-developed database vulnerability testing tool, we have identified the above-mentioned vulnerabilities in TiDB that may lead to database logic error.

@r33s3n6 r33s3n6 added the type/bug The issue is confirmed as a bug. label Apr 29, 2024
@ti-chi-bot ti-chi-bot added affects-8.1 This bug affects the 8.1.x(LTS) versions. and removed may-affects-8.1 labels May 3, 2024
@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-5.4

@ti-chi-bot ti-chi-bot bot added affects-5.4 This bug affects the 5.4.x(LTS) versions. and removed may-affects-5.4 This bug maybe affects 5.4.x versions. labels May 6, 2024
@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-6.1

@ti-chi-bot ti-chi-bot bot added affects-6.1 This bug affects the 6.1.x(LTS) versions. and removed may-affects-6.1 labels May 6, 2024
@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-6.5

@ti-chi-bot ti-chi-bot bot added affects-6.5 This bug affects the 6.5.x(LTS) versions. and removed may-affects-6.5 labels May 6, 2024
@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-7.1

@ti-chi-bot ti-chi-bot bot added the affects-7.1 This bug affects the 7.1.x(LTS) versions. label May 6, 2024
@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-7.5

@ti-chi-bot ti-chi-bot bot added affects-7.5 This bug affects the 7.5.x(LTS) versions. and removed may-affects-7.1 may-affects-7.5 labels May 6, 2024
@SeaRise
Copy link
Contributor

SeaRise commented May 7, 2024

/assign @SeaRise

@SeaRise
Copy link
Contributor

SeaRise commented May 9, 2024

Reproduced on a single node, after creating the table and inserting data, wait a while.
Also, the plan shows it's unrelated to the number of TiKV nodes.

+--------------------------------------+---------+---------+-----------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------+---------+
| id                                   | estRows | actRows | task      | access object | execution info                                                                                                                                                                                                                                                                                                                                                                                                                | operator info                                                                                                                                                                                                                                                                                              | memory    | disk    |
+--------------------------------------+---------+---------+-----------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------+---------+
| Union_14                             | 512.00  | 512     | root      |               | time:3.69ms, loops:7, RU:2.539922                                                                                                                                                                                                                                                                                                                                                                                             |                                                                                                                                                                                                                                                                                                            | N/A       | N/A     |
| ├─Projection_17                      | 4.00    | 4       | root      |               | time:931.8µs, loops:2, Concurrency:OFF                                                                                                                                                                                                                                                                                                                                                                                        | test.t_j.c_tk->Column#21, test.t_j.c_epy6r_00o8->Column#22, test.t_j.c_tk->Column#23, test.t_j.c_x49cz->Column#24, test.t_j.c_qp->Column#25                                                                                                                                                                | 1.73 KB   | N/A     |
| │ └─TableReader_20                   | 4.00    | 4       | root      |               | time:926.1µs, loops:2, cop_task: {num: 1, max: 848.6µs, proc_keys: 4, tot_proc: 120.7µs, tot_wait: 94.2µs, copr_cache_hit_ratio: 0.00, build_task_duration: 11.7µs, max_distsql_concurrency: 1}, rpc_info:{Cop:{num_rpc:1, total_time:815.6µs}}                                                                                                                                                                               | data:TableFullScan_19                                                                                                                                                                                                                                                                                      | 423 Bytes | N/A     |
| │   └─TableFullScan_19               | 4.00    | 4       | cop[tikv] | table:ref_0   | tikv_task:{time:0s, loops:1}, scan_detail: {total_process_keys: 4, total_process_keys_size: 245, total_keys: 5, get_snapshot_time: 44.8µs, rocksdb: {delete_skipped_count: 4, key_skipped_count: 8, block: {cache_hit_count: 2}}}, time_detail: {total_process_time: 120.7µs, total_wait_time: 94.2µs, tikv_wall_time: 397.6µs}                                                                                               | keep order:false                                                                                                                                                                                                                                                                                           | N/A       | N/A     |
| └─Projection_30                      | 508.00  | 508     | root      |               | time:3.51ms, loops:6, Concurrency:OFF                                                                                                                                                                                                                                                                                                                                                                                         | cast(Column#13, text CHARACTER SET utf8mb4 COLLATE utf8mb4_bin)->Column#21, Column#15->Column#22, test.t_ufims7.c__w->Column#23, test.t_ufims7.c_h20voo1018->Column#24, test.t_ufims7.c_a4f1v5d3q2->Column#25                                                                                              | 14.2 KB   | N/A     |
|   └─Shuffle_37                       | 508.00  | 508     | root      |               | time:3.45ms, loops:6, ShuffleConcurrency:5                                                                                                                                                                                                                                                                                                                                                                                    | execution info: concurrency:5, data sources:[Projection_33]                                                                                                                                                                                                                                                | N/A       | N/A     |
|     └─Window_31                      | 508.00  | 508     | root      |               | time:17.2ms, loops:10                                                                                                                                                                                                                                                                                                                                                                                                         | var_pop(<nil>)->Column#15 over(partition by test.t_ufims7.c_qbi13s order by test.t_ufims7.c_qbi13s, test.t_ufims7.c_a4f1v5d3q2, test.t_ufims7.c_yhcnkidi8, test.t_ufims7.c_qm, test.t_ufims7.c_h20voo1018, test.t_ufims7.c__w, test.t_ufims7.c_xxn6xl2q range between unbounded preceding and current row) | N/A       | N/A     |
|       └─Sort_36                      | 508.00  | 508     | root      |               | time:16.6ms, loops:10                                                                                                                                                                                                                                                                                                                                                                                                         | test.t_ufims7.c_qbi13s, test.t_ufims7.c_qbi13s, test.t_ufims7.c_a4f1v5d3q2, test.t_ufims7.c_yhcnkidi8, test.t_ufims7.c_qm, test.t_ufims7.c_h20voo1018, test.t_ufims7.c__w, test.t_ufims7.c_xxn6xl2q                                                                                                        | 14.5 KB   | 0 Bytes |
|         └─ShuffleReceiver_38         | 508.00  | 508     | root      |               | time:16.3ms, loops:10                                                                                                                                                                                                                                                                                                                                                                                                         |                                                                                                                                                                                                                                                                                                            | N/A       | N/A     |
|           └─Projection_33            | 508.00  | 508     | root      |               | time:3.12ms, loops:2, Concurrency:OFF                                                                                                                                                                                                                                                                                                                                                                                         | 1->Column#13, test.t_ufims7.c__w, test.t_ufims7.c_h20voo1018, test.t_ufims7.c_a4f1v5d3q2, test.t_ufims7.c_qbi13s, test.t_ufims7.c_yhcnkidi8, test.t_ufims7.c_qm, test.t_ufims7.c_xxn6xl2q                                                                                                                  | 36.4 KB   | N/A     |
|             └─TableReader_35         | 508.00  | 508     | root      |               | time:3.1ms, loops:2, cop_task: {num: 2, max: 1.64ms, min: 1.38ms, avg: 1.51ms, p95: 1.64ms, max_proc_keys: 284, p95_proc_keys: 284, tot_proc: 1.39ms, tot_wait: 212.4µs, copr_cache_hit_ratio: 0.00, build_task_duration: 7.68µs, max_distsql_concurrency: 1}, rpc_info:{Cop:{num_rpc:2, total_time:2.96ms}}                                                                                                                  | data:TableFullScan_34                                                                                                                                                                                                                                                                                      | 31.1 KB   | N/A     |
|               └─TableFullScan_34     | 508.00  | 508     | cop[tikv] | table:ref_2   | tikv_task:{proc max:1ms, min:0s, avg: 500µs, p80:1ms, p95:1ms, iters:7, tasks:2}, scan_detail: {total_process_keys: 508, total_process_keys_size: 39783, total_keys: 902, get_snapshot_time: 126µs, rocksdb: {delete_skipped_count: 421, key_skipped_count: 1711, block: {cache_hit_count: 4}}}, time_detail: {total_process_time: 1.39ms, total_wait_time: 212.4µs, total_kv_read_wall_time: 1ms, tikv_wall_time: 1.93ms}    | keep order:false, stats:pseudo                                                                                                                                                                                                                                                                             | N/A       | N/A     |
+--------------------------------------+---------+---------+-----------+---------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+-----------+---------+

The result is correct after setting tidb_executor_concurrency to 1, which should be a concurrency issue in TiDB.
And then setting tidb_window_concurrency to 100, the issue occurred, likely related to window shuffle.
Replacing var_pop with count also resulted in an error, indicating the issue is unrelated to var_pop.

@SeaRise
Copy link
Contributor

SeaRise commented May 9, 2024

Simplified case

use test;
drop table if exists t1;
create table t1 (cc1 int,cc2 text);
insert into t1 values (1, 'aaaa'),(2, 'bbbb'),(3, 'cccc');

drop table if exists t2;
create table t2 (cc1 int,cc2 text,primary key(cc1));
insert into t2 values (2, '2');

set tidb_executor_concurrency = 1;
set tidb_window_concurrency = 100;
SELECT DISTINCT cc2, cc2, cc1 FROM t2 UNION ALL SELECT count(1) over (partition by cc1), cc2, cc1 FROM t1;

+------+------+------+
| cc2  | cc2  | cc1  |
+------+------+------+
| 2    | 2    |    2 |
| 1    | cccc |    3 |
| 1    | bbbb |    2 |
| aaaa | aaaa |    1 |
+------+------+------+

@winoros
Copy link
Member

winoros commented Aug 1, 2024

Simplified case

use test;
drop table if exists t1;
create table t1 (cc1 int,cc2 text);
insert into t1 values (1, 'aaaa'),(2, 'bbbb'),(3, 'cccc');

drop table if exists t2;
create table t2 (cc1 int,cc2 text,primary key(cc1));
insert into t2 values (2, '2');

set tidb_executor_concurrency = 1;
set tidb_window_concurrency = 100;
SELECT DISTINCT cc2, cc2, cc1 FROM t2 UNION ALL SELECT count(1) over (partition by cc1), cc2, cc1 FROM t1;

+------+------+------+
| cc2  | cc2  | cc1  |
+------+------+------+
| 2    | 2    |    2 |
| 1    | cccc |    3 |
| 1    | bbbb |    2 |
| aaaa | aaaa |    1 |
+------+------+------+

It will get correct result by
image

The AvoidColumnEvaluator is introduced as a short-term workaround. I think it's time to give a general solution for the chunk reusing.

@winoros
Copy link
Member

winoros commented Aug 1, 2024

image
image
image

@winoros
Copy link
Member

winoros commented Aug 1, 2024

The columnEvaluator is introduced for reusing the chunk when a column is projected multiple times.
e.g. The project{expr: [a, a, a], output cols: [a1, a2, a3]} of select a as a1, a as a2, a as a3 from t will use one chunk column of a to represent the three output chunk columns.

But the reusing becomes a problem when the projection is related to union.

@SeaRise
Copy link
Contributor

SeaRise commented Aug 1, 2024

@SeaRise Why is it a planner issue? Can you please point out where the plan is wrong? Thanks.

It was found that the aggregation elimination rule introduced an error during the construction of the plan, so I think this is an issue about planner.

@winoros
Copy link
Member

winoros commented Aug 1, 2024

@SeaRise Why is it a planner issue? Can you please point out where the plan is wrong? Thanks.

It was found that the aggregation elimination rule introduced an error during the construction of the plan, so I think this is an issue about planner.

I'm not good at the execution techs. Maybe one way is to introduce copy-on-write for chunk reuse? Then planner don't need to consider this execution option AvoidColumnEvaluator on the planner side.

@SeaRise
Copy link
Contributor

SeaRise commented Aug 2, 2024

@SeaRise Why is it a planner issue? Can you please point out where the plan is wrong? Thanks.

It was found that the aggregation elimination rule introduced an error during the construction of the plan, so I think this is an issue about planner.

I'm not good at the execution techs. Maybe one way is to introduce copy-on-write for chunk reuse? Then planner don't need to consider this execution option AvoidColumnEvaluator on the planner side.

@XuHuaiyu What do you think?

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Aug 6, 2024

Why the first child of union is a hashagg instead of a projection? @winoros

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Aug 6, 2024

The result could be correct after the following modification.

diff --git a/pkg/planner/core/rule_eliminate_projection.go b/pkg/planner/core/rule_eliminate_projection.go
index c323fb20cd..11d4c13925 100644
--- a/pkg/planner/core/rule_eliminate_projection.go
+++ b/pkg/planner/core/rule_eliminate_projection.go
@@ -112,13 +112,17 @@ func doPhysicalProjectionElimination(p base.PhysicalPlan) base.PhysicalPlan {
                return p
        }
        child := p.Children()[0]
-       if childProj, ok := child.(*PhysicalProjection); ok {
+       childProj, ok := child.(*PhysicalProjection)
+       if ok {
                // when current projection is an empty projection(schema pruned by column pruner), no need to reset child's schema
                // TODO: avoid producing empty projection in column pruner.
                if p.Schema().Len() != 0 {
                        childProj.SetSchema(p.Schema())
                }
        }
+       if proj.AvoidColumnEvaluator {
+               childProj.AvoidColumnEvaluator = true
+       }
        for i, col := range p.Schema().Columns {
                if p.SCtx().GetSessionVars().StmtCtx.ColRefFromUpdatePlan.Has(int(col.UniqueID)) && !child.Schema().Columns[i].Equal(nil, col) {
                        return p

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Aug 6, 2024

related: #19027

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Aug 7, 2024

Another approach to fix this issue:
Adding a postOptimize rule, if a projection operator is a child of Union, set the AvoidColumnEvaluator to true.

ti-chi-bot bot pushed a commit that referenced this issue Aug 7, 2024
hawkingrei pushed a commit to hawkingrei/tidb that referenced this issue Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. fuzz/randomtest impact/wrong-result severity/major sig/planner SIG: Planner type/bug The issue is confirmed as a bug.
Projects
None yet
8 participants