statistics: fix PK column TopN not loading when init stats #37552

xuyifangreeneyes · 2022-09-01T12:33:13Z

What problem does this PR solve?

Issue Number: close #37548

Problem Summary:

What is changed and how it works?

Since we have topn for PK in stats ver2, when init stats, we need to load topn for PK.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
Test as primary key column's Histogram and TopN are not loaded after restarting TiDB #37548 suggests.
No code

Side effects

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

Documentation

Release note

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

None

ti-chi-bot · 2022-09-01T12:33:15Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

statistics/handle/bootstrap.go

qw4990 · 2023-02-16T10:53:12Z

statistics/handle/bootstrap.go

 }

 func (h *Handle) initStatsTopN(cache *statsCache) error {
 	ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnStats)
-	sql := "select HIGH_PRIORITY table_id, hist_id, value, count from mysql.stats_top_n where is_index = 1"
+	sql := "select HIGH_PRIORITY table_id, is_index, hist_id, value, count from mysql.stats_top_n"


I see that we only load Histograms into memory for indexes when starting TiDB, so should we keep the TopN corresponding with Histograms here? (only load index-TopN and skip column-TopN)

Yes, we should do that, except for PK. If the column is PK, we load its histogram and TopN into memory, otherwise we don't.

Maybe we can join information_schema.columns to get entries whose is_index=1 or COLUMN_KEY='PRI'?

time-and-fate · 2023-02-16T10:36:24Z

statistics/handle/bootstrap.go

 }

 func (h *Handle) initStatsTopN(cache *statsCache) error {
 	ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnStats)
-	sql := "select HIGH_PRIORITY table_id, hist_id, value, count from mysql.stats_top_n where is_index = 1"
+	sql := "select HIGH_PRIORITY table_id, is_index, hist_id, value, count from mysql.stats_top_n"


Seems we will read many unneeded data (non-PK columns' TopN). Is this acceptable?

I think we need to avoid reading unnecessary data. It sometimes takes about 20 minutes to init stats when the cluster is under high pressure, and the modification will make it worse. Maybe we need a more efficient way to do this.

time-and-fate · 2023-02-16T11:03:52Z

statistics/handle/bootstrap.go

+			idx.TopN.AppendTopN(data, row.GetUint64(4))
+		} else {
+			col, ok := table.Columns[row.GetInt64(2)]
+			if !ok || col.Info == nil || !mysql.HasPriKeyFlag(col.Info.GetFlag()) || (col.CMSketch == nil && col.StatsVer <= statistics.Version1) {


Looks like this fix should also apply to stats ver1🤔

I did some simple experiments and believe this bug also affects stats ver1, so probably this fix should also apply to stats ver1.

Got it. I will make the fix apply to stats ver1.

The condititon col.CMSketch == nil && col.StatsVer <= statistics.Version1 is copied from the condition of loading index TopN(idx.CMSketch == nil && idx.StatsVer <= statistics.Version1), which I don't fully understand by now. The condition is from #14421 and #24623. I will take a deep look into it.

time-and-fate · 2023-02-16T13:52:32Z

Suggested title: fix PK column TopN not loading when init stats

Co-authored-by: Yuanjia Zhang <qw4990@163.com>

ti-chi-bot · 2023-04-22T18:12:14Z

PR needs rebase.

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 kubernetes/test-infra repository.

ti-chi-bot · 2023-07-06T07:55:15Z

@xuyifangreeneyes: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`c359a4e`	link	true	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

ti-chi-bot · 2023-07-06T07:55:17Z

@xuyifangreeneyes: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`c359a4e`	link	true	`/test pull-br-integration-test`

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 kubernetes/test-infra repository. I understand the commands that are listed here.

codecov · 2023-11-17T16:37:31Z

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 73.5833%. Comparing base (f9e1845) to head (c359a4e).
Report is 4160 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #37552        +/-   ##
================================================
+ Coverage   73.4684%   73.5833%   +0.1148%     
================================================
  Files          1124       1125         +1     
  Lines        357948     358134       +186     
================================================
+ Hits         262979     263527       +548     
+ Misses        77937      77584       -353     
+ Partials      17032      17023         -9

hawkingrei · 2024-06-28T06:11:00Z

It has been fixed by #53298.

fix loading column stats when init stats

7cc1daa

Merge branch 'master' into fix-37548

4a9241e

xuyifangreeneyes marked this pull request as ready for review January 10, 2023 08:18

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2023

xuyifangreeneyes and others added 2 commits February 16, 2023 13:31

fix

532f90c

Merge branch 'master' into fix-37548

01f55bc

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2023

add test

4f0128a

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 16, 2023

xuyifangreeneyes added 2 commits February 16, 2023 16:13

upd

03fb222

upd

4f8d3e4

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 16, 2023

fmt

12e93b9

xuyifangreeneyes requested review from time-and-fate and qw4990 February 16, 2023 08:37

qw4990 reviewed Feb 16, 2023

View reviewed changes

statistics/handle/bootstrap.go Outdated Show resolved Hide resolved

qw4990 reviewed Feb 16, 2023

View reviewed changes

time-and-fate reviewed Feb 16, 2023

View reviewed changes

xuyifangreeneyes changed the title ~~statistics: fix loading column stats when init stats~~ statistics: fix PK column TopN not loading when init stats Feb 20, 2023

Update statistics/handle/bootstrap.go

c359a4e

Co-authored-by: Yuanjia Zhang <qw4990@163.com>

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2023

ti-chi-bot added needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. labels May 16, 2024

ti-chi-bot added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Jun 25, 2024

hawkingrei closed this Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: fix PK column TopN not loading when init stats #37552

statistics: fix PK column TopN not loading when init stats #37552

xuyifangreeneyes commented Sep 1, 2022 •

edited

Loading

ti-chi-bot commented Sep 1, 2022

qw4990 Feb 16, 2023 •

edited

Loading

xuyifangreeneyes Feb 20, 2023

chrysan Feb 21, 2023

time-and-fate Feb 16, 2023 •

edited

Loading

xuyifangreeneyes Feb 20, 2023

time-and-fate Feb 16, 2023

time-and-fate Feb 16, 2023

xuyifangreeneyes Feb 20, 2023

xuyifangreeneyes Feb 20, 2023

time-and-fate commented Feb 16, 2023

ti-chi-bot bot commented Apr 22, 2023

ti-chi-bot bot commented Jul 6, 2023

ti-chi-bot commented Jul 6, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

hawkingrei commented Jun 28, 2024

statistics: fix PK column TopN not loading when init stats #37552

statistics: fix PK column TopN not loading when init stats #37552

Conversation

xuyifangreeneyes commented Sep 1, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 1, 2022

qw4990 Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

xuyifangreeneyes Feb 20, 2023

Choose a reason for hiding this comment

chrysan Feb 21, 2023

Choose a reason for hiding this comment

time-and-fate Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

xuyifangreeneyes Feb 20, 2023

Choose a reason for hiding this comment

time-and-fate Feb 16, 2023

Choose a reason for hiding this comment

time-and-fate Feb 16, 2023

Choose a reason for hiding this comment

xuyifangreeneyes Feb 20, 2023

Choose a reason for hiding this comment

xuyifangreeneyes Feb 20, 2023

Choose a reason for hiding this comment

time-and-fate commented Feb 16, 2023

ti-chi-bot bot commented Apr 22, 2023

ti-chi-bot bot commented Jul 6, 2023

ti-chi-bot commented Jul 6, 2023

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

hawkingrei commented Jun 28, 2024

xuyifangreeneyes commented Sep 1, 2022 •

edited

Loading

qw4990 Feb 16, 2023 •

edited

Loading

time-and-fate Feb 16, 2023 •

edited

Loading

codecov bot commented Nov 17, 2023 •

edited

Loading