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

*: enable the predicate columns feature by default | tidb-test=pr/2361 #54440

Merged

Conversation

hi-rustin
Copy link
Member

@hi-rustin hi-rustin commented Jul 4, 2024

What problem does this PR solve?

Issue Number: ref #53567

Problem Summary:

What changed and how does it work?

I have enabled the predicate columns feature by default. However, this caused a lot of test cases to break because we updated them to collect predicate columns' statistics. Here's how I tackled updating these cases:

  1. For unit tests, especially those related to statistics, I added a new function called TriggerPredicateColumnsCollection to help collect the predicate columns. This allowed us to fix those tests.
  2. For test cases in other modules, I first tried adding the index. If adding the index fixed the test without changing the test output, then I simplified it by adding the index. Otherwise, I used the 'all columns' syntax to ensure that this update wouldn't change the test logic for those cases.

We need to find a way to collect the predicate columns without having to wait for the stats usage dumping process during integration tests. I'll address this after completing all the testing work for this feature. In the meantime, we can always use all the columns as a workaround.

At the same time, I added the upgradeToVer210 function to ensure that we don't change the behavior of columns collecting(all columns) if the users upgrade the TiDB cluster from a lower version.

You can find the benchmark result here: http://perf.pingcap.net/d/iarAjGo7z/benchbot-overview?orgId=1&var-email=example%40pingcap.cn&var-testbed=benchbot-oltp-001-tps-7598610-1-822&from=now-30d&to=now (PingCAP internally only) No regression from the benchmark result.

I will do more tests after this PR is merged.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test
  • No need to test
    • I checked and no code files have been changed.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

统计信息收集默认只收集 predicate 列
Analyze only analyzes predicate columns by default

@ti-chi-bot ti-chi-bot bot added release-note size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 4, 2024
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 56.8086%. Comparing base (05c8119) to head (356a7b5).
Report is 9 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #54440         +/-   ##
=================================================
- Coverage   72.7895%   56.8086%   -15.9810%     
=================================================
  Files          1549       1665        +116     
  Lines        436285     606397     +170112     
=================================================
+ Hits         317570     344486      +26916     
- Misses        99067     238247     +139180     
- Partials      19648      23664       +4016     
Flag Coverage Δ
integration 37.9247% <0.0000%> (?)
unit 71.7970% <40.0000%> (+0.0022%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 52.9656% <ø> (ø)
parser ∅ <ø> (∅)
br 62.8103% <ø> (+16.9496%) ⬆️

@ti-chi-bot ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 4, 2024
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch from b884856 to 2f38ff2 Compare July 5, 2024 10:14
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 8, 2024
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch 2 times, most recently from 8d81dda to 5a7a370 Compare July 10, 2024 06:59
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2024
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch from 5a7a370 to a275d36 Compare July 11, 2024 05:39
@hi-rustin hi-rustin changed the title *: enable the predicate columns feature by default *: enable the predicate columns feature by default | tidb-test=pr/2361 Jul 11, 2024
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch from 6faee49 to 40ad678 Compare July 12, 2024 04:03
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch 2 times, most recently from 1b7d9a1 to 4bb5e3c Compare July 12, 2024 05:10
@hi-rustin hi-rustin force-pushed the rustin-patch-enable-predicate-columns branch from 4bb5e3c to f475fd9 Compare July 12, 2024 05:21
@hi-rustin
Copy link
Member Author

Test locally:

Upgrade

  1. Start a low version TiDB: tiup cluster deploy upstream v7.5.0 upstream.yaml -p && tiup cluster start upstream
  2. Patch the cluster: tiup cluster patch upstream ../../../tidb/bin/tidb-hotfix-linux-amd64.tar.gz -N 10.2.6.161:4000,10.2.6.253:4000,10.2.7.158:4000 -R tidb
  3. Check the @@tidb_analyze_column_options:
Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MySQL [(none)]> select @@tidb_analyze_column_options;
+-------------------------------+
| @@tidb_analyze_column_options |
+-------------------------------+
| ALL                           |
+-------------------------------+
1 row in set (0.00 sec)

New cluster:

  1. Start a new cluster with the patch: tiup playground nightly --db.binpath /Volumes/t7/code/tidb/bin/tidb-server
  2. Check the @@tidb_analyze_column_options:
mysql> select @@tidb_analyze_column_options;
+-------------------------------+
| @@tidb_analyze_column_options |
+-------------------------------+
| PREDICATE                     |
+-------------------------------+
1 row in set (0.01 sec)

@hi-rustin hi-rustin requested review from qw4990 and winoros July 12, 2024 05:37
@hi-rustin hi-rustin requested review from Leavrth and ywqzzy July 12, 2024 05:37
Copy link
Member Author

@hi-rustin hi-rustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔢 Self-check (PR reviewed by myself and ready for feedback.)

@hi-rustin hi-rustin requested review from GMHDBJD and removed request for ywqzzy July 12, 2024 06:31
Copy link

ti-chi-bot bot commented Jul 12, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-07-12 06:45:19.621811893 +0000 UTC m=+599216.857046003: ☑️ agreed by Leavrth.
  • 2024-07-12 07:19:25.991816136 +0000 UTC m=+601263.227050246: ☑️ agreed by qw4990.

Copy link
Contributor

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for DDL part

@easonn7
Copy link

easonn7 commented Jul 12, 2024

/approve

Copy link

ti-chi-bot bot commented Jul 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: easonn7, GMHDBJD, Leavrth, qw4990

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Jul 12, 2024
@hi-rustin
Copy link
Member Author

/retest

@ti-chi-bot ti-chi-bot bot merged commit e5a8a23 into pingcap:master Jul 12, 2024
36 checks passed
@hi-rustin hi-rustin deleted the rustin-patch-enable-predicate-columns branch July 12, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm release-note sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants