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

var_pop on constant values may produce wrong result. #52981

Open
r33s3n6 opened this issue Apr 29, 2024 · 14 comments
Open

var_pop on constant values may produce wrong result. #52981

r33s3n6 opened this issue Apr 29, 2024 · 14 comments

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 calculates the var_pop value of the constant expression 604102521670485727, and the result should be 0.

3. What did you see instead (Required)

However, the statement results are unstable; in both the multi-node and single-node versions, there are some 0 and non-zero values.

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.

@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

The plan looks all right, var_pop is executed in tidb side, the implementation of var_pop seems incorrect:

mysql> source error.sql.txt
+----------------------------------------+---------+--------------+---------------+------------------------------------------------------------------------------------------------------------------------------+
| id                                     | estRows | task         | access object | operator info                                                                                                                |
+----------------------------------------+---------+--------------+---------------+------------------------------------------------------------------------------------------------------------------------------+
| HashAgg_15                             | 249.60  | root         |               | group by:Column#17, funcs:var_pop(6.041025216704858e+17)->Column#18                                                          |
| └─TableReader_38                       | 249.60  | root         |               | MppVersion: 2, data:ExchangeSender_37                                                                                        |
|   └─ExchangeSender_37                  | 249.60  | mpp[tiflash] |               | ExchangeType: PassThrough                                                                                                    |
|     └─Projection_33                    | 249.60  | mpp[tiflash] |               | Column#17                                                                                                                    |
|       └─HashAgg_34                     | 249.60  | mpp[tiflash] |               | group by:test.t_ufims7.c__w, funcs:sum(Column#19)->Column#17, stream_count: 4                                                |
|         └─ExchangeReceiver_36          | 249.60  | mpp[tiflash] |               | stream_count: 4                                                                                                              |
|           └─ExchangeSender_35          | 249.60  | mpp[tiflash] |               | ExchangeType: HashPartition, Compression: FAST, Hash Cols: [name: test.t_ufims7.c__w, collate: utf8mb4_bin], stream_count: 4 |
|             └─HashAgg_20               | 249.60  | mpp[tiflash] |               | group by:Column#22, funcs:count(Column#21)->Column#19                                                                        |
|               └─Projection_42          | 312.00  | mpp[tiflash] |               | cast(test.t_ufims7.c_yhcnkidi8, bigint(22) BINARY)->Column#21, test.t_ufims7.c__w->Column#22                                 |
|                 └─TableFullScan_32     | 312.00  | mpp[tiflash] | table:ref_4   | keep order:false, stats:pseudo                                                                                               |
+----------------------------------------+---------+--------------+---------------+------------------------------------------------------------------------------------------------------------------------------+                                                                                                               

@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 5.4.x 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

@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-6.5

@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-7.1

@yibin87
Copy link
Contributor

yibin87 commented May 6, 2024

/label affects-7.5

@SeaRise
Copy link
Contributor

SeaRise commented May 7, 2024

/assign @SeaRise

@SeaRise
Copy link
Contributor

SeaRise commented May 7, 2024

Simplified case

use test;
drop table if exists test;
create table test (c int);
insert into test values(0),(0),(0),(0),(0),(0);
SELECT var_pop(604102521670485727) FROM test;

// mysql
mysql> SELECT var_pop(604102521670485727) FROM test;
+-----------------------------+
| var_pop(604102521670485727) |
+-----------------------------+
|                           0 |
+-----------------------------+

// tidb-v8.2.0-alpha
mysql> SELECT var_pop(604102521670485727) FROM test;
+-----------------------------+
| var_pop(604102521670485727) |
+-----------------------------+
|          1456.3555555555556 |
+-----------------------------+

@SeaRise
Copy link
Contributor

SeaRise commented May 7, 2024

The correct result of 3020512608352429000 + 604102521670485800 is 3624615130022914800
However, when using float64, the results are incorrect in both Golang and C++.

  • golang
    a1801a91-17ee-4dca-aa0d-6c810b08ad17
  • c++
    2415d65e-6b6f-4e0b-b31b-56d9bc27c9a7

@SeaRise
Copy link
Contributor

SeaRise commented May 8, 2024

@SeaRise
Copy link
Contributor

SeaRise commented May 8, 2024

/label severity/minor

Copy link

ti-chi-bot bot commented May 8, 2024

@SeaRise: The label(s) severity/minor cannot be applied. These labels are supported: fuzz/sqlancer, challenge-program, compatibility-breaker, first-time-contributor, contribution, good first issue, correctness, duplicate, proposal, security, ok-to-test, needs-ok-to-test, needs-more-info, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.5, needs-cherry-pick-release-7.1, needs-cherry-pick-release-7.5, needs-cherry-pick-release-8.1, affects-5.4, affects-6.1, affects-6.5, affects-7.1, affects-7.5, affects-8.1, may-affects-5.4, may-affects-6.1, may-affects-6.5, may-affects-7.1, may-affects-7.5, may-affects-8.1.

In response to this:

/label severity/minor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@SeaRise
Copy link
Contributor

SeaRise commented May 8, 2024

/severity minor

@SeaRise
Copy link
Contributor

SeaRise commented May 8, 2024

The root cause is an issue with float64 addition precision. As mentioned in the previous comment, the sum of 3020512608352429000 and 604102521670485800 is incorrect in Golang, which leads to an incorrect result for var_pop.
After examining MySQL's implementation, it also uses float64 for var_pop, but with a different algorithm than TiDB. Still, it's theoretically possible to find a case that yields an incorrect result in MySQL.
The severity is downgraded to minor due to the complex potential fix, rare usage mentioned in the issue, and the non-precision nature of var_pop itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants