statistics: correct behavior of non-lite InitStats and stats sync load of no stats column #57803

winoros · 2024-11-28T12:30:05Z

What problem does this PR solve?

Issue Number: close #57804

Problem Summary:

What changed and how does it work?

You can regard this pull request as a totally rewrite of #53399 and #53298

This pull reduced the possible stats state after the stats initialization finished. Now we'll get the same memory objects after a non-concurrent or current non-lite load.

Then use the unified status to make the ColumnIsLoadNeeded correct when one column even has no record in mysql.stats_histograms.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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

Release note

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

None

codecov · 2024-11-28T12:50:44Z

Codecov Report

Attention: Patch coverage is 86.44068% with 8 lines in your changes missing coverage. Please review.

Project coverage is 73.7086%. Comparing base (cc59ab0) to head (6d003ca).
Report is 18 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57803        +/-   ##
================================================
+ Coverage   72.8626%   73.7086%   +0.8459%     
================================================
  Files          1677       1709        +32     
  Lines        464331     476385     +12054     
================================================
+ Hits         338324     351137     +12813     
+ Misses       105118     103599      -1519     
- Partials      20889      21649       +760

Flag	Coverage Δ
integration	`46.1183% <27.1186%> (?)`
unit	`72.2994% <86.4406%> (+0.0366%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.1196% <ø> (-0.5911%)`	⬇️

winoros · 2024-11-28T21:13:04Z

pkg/statistics/handle/handletest/statstest/stats_test.go

-	if isConcurrency {
-		require.Equal(t, uint8(0x3), table0.GetIdx(1).LastAnalyzePos.GetBytes()[0])
-		require.Equal(t, uint8(0x3), table0.GetIdx(2).LastAnalyzePos.GetBytes()[0])
-	} else {
-		require.Equal(t, uint8(0x33), table0.GetCol(1).LastAnalyzePos.GetBytes()[0])
-		require.Equal(t, uint8(0x33), table0.GetCol(2).LastAnalyzePos.GetBytes()[0])
-		require.Equal(t, uint8(0x33), table0.GetCol(3).LastAnalyzePos.GetBytes()[0])
-		require.Equal(t, uint8(0x3), table0.GetIdx(1).LastAnalyzePos.GetBytes()[0])
-		require.Equal(t, uint8(0x3), table0.GetIdx(2).LastAnalyzePos.GetBytes()[0])
-	}


In fact, we should not distinguish between the single thread init and the concurrent init.
This pr makes them consistent.

winoros · 2024-11-28T21:16:16Z

pkg/statistics/handle/handletest/statstest/stats_test.go

+	require.True(t, table0.GetCol(1).IsAllEvicted())
+	require.True(t, table0.GetCol(2).IsAllEvicted())
+	require.True(t, table0.GetCol(3).IsAllEvicted())
+	require.True(t, !table0.GetCol(4).IsStatsInitialized())
+	require.True(t, table0.GetCol(5).IsStatsInitialized())


column 1/2/3 is analyzed. col4 is not analyzed and has no record in storage.
col5 is created by add column default value so it has stats though it's not analyzed.

winoros · 2024-11-28T21:49:39Z

pkg/statistics/handle/handletest/initstats/load_stats_test.go

@@ -88,7 +88,7 @@ func testConcurrentlyInitStats(t *testing.T) {
 		tk.MustQuery(fmt.Sprintf("explain select * from t%v where b = 1", i)).CheckNotContain("pseudo")
 	}
 	for i := 1; i < 10; i++ {
-		tk.MustQuery(fmt.Sprintf("explain select * from t%v where c = 1", i)).CheckNotContain("pseudo")


pk = 1 will result in PointGet. Will not trigger the stats load.

ti-chi-bot · 2024-11-29T01:19:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei

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:

~~pkg/statistics/OWNERS~~ [hawkingrei]

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 · 2024-11-29T01:19:10Z

[LGTM Timeline notifier]

Timeline:

2024-11-29 01:19:10.185011604 +0000 UTC m=+772137.804666119: ☑️ agreed by hawkingrei.

Rustin170506

Could you please summarize the changes made in your PR description? It's a bit difficult to understand what is happening in the PR.

Rustin170506 · 2024-12-02T07:54:53Z

pkg/statistics/handle/bootstrap.go

@@ -619,6 +638,15 @@ func (*Handle) initStatsBuckets4Chunk(cache statstypes.StatsCache, iter *chunk.I
 	}
 }

+func initStatsBucketsSQLGen(isPaging bool) string {
+	selectPrefix := "select /*+ ORDER_INDEX(mysql.stats_buckets,tbl) */ HIGH_PRIORITY table_id, is_index, hist_id, count, repeats, lower_bound, upper_bound, ndv from mysql.stats_buckets where is_index=1"


This seems different from the old one. Could you please explain why we need is_index=1 here?

Note that we've already not read the column's topn into memory. So reading the buckets here is a useless operation since the stats have already broken.

Rustin170506 · 2024-12-02T07:55:36Z

pkg/statistics/handle/bootstrap.go

 func (h *Handle) initStatsTopN(cache statstypes.StatsCache, totalMemory uint64) error {
-	sql := "select /*+ ORDER_INDEX(mysql.stats_top_n,tbl)*/  HIGH_PRIORITY table_id, hist_id, value, count from mysql.stats_top_n where is_index = 1 order by table_id"
+	sql := initStatsTopNSQLGen(false)


This seems different from the old one. Could you please explain why we need to remove is_index=1 here?

the is_index=1 is not removed. It's aligned with the non-concurrent version. This is to make the concurrent one and the non-concurrent one the same.

Rustin170506 · 2024-12-02T07:56:06Z

pkg/statistics/handle/bootstrap.go

@@ -285,7 +298,7 @@ func (h *Handle) initStatsHistogramsLite(ctx context.Context, cache statstypes.S
 }

 func (h *Handle) initStatsHistograms(is infoschema.InfoSchema, cache statstypes.StatsCache) error {
-	sql := "select  /*+ ORDER_INDEX(mysql.stats_histograms,tbl)*/ HIGH_PRIORITY table_id, is_index, hist_id, distinct_count, version, null_count, cm_sketch, tot_col_size, stats_ver, correlation, flag, last_analyze_pos from mysql.stats_histograms order by table_id"
+	sql := initStatsHistogramsSQLGen(false)


This seems different from the old one. Could you please explain why we need to remove is_index=1 here?

If you search the caller of the initStatsHistogramSQLGen you will see that now initStatsHistogramsLite, initStatsHistograms, and initStatsHistogramsConcurrency use the same SQL gen.
Actually, the SQL is not changed for initStatsHistogramsLite and initStatsHistograms. It just aligned the initStatsHistogramsConcurrency with the other two.

pkg/statistics/table.go

Co-authored-by: Rustin <tech@rustin.me>

tiprow · 2024-12-02T12:00:39Z

@winoros: 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
fast_test_tiprow	`6d003ca`	link	true	`/test fast_test_tiprow`

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-sigs/prow repository. I understand the commands that are listed here.

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2024

winoros force-pushed the loadneeded-fix branch from 296b172 to d201740 Compare November 28, 2024 12:34

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2024

statistics: try to eliminate the unnecessary log for sync load

16936a5

winoros force-pushed the loadneeded-fix branch from d201740 to 16936a5 Compare November 28, 2024 14:40

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 28, 2024

winoros changed the title ~~statistics: try to eliminate the unnecessary log for sync load~~ statistics: try to eliminate the unnecessary log for sync load | tidb-test=pr/2435 Nov 28, 2024

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Nov 28, 2024

winoros force-pushed the loadneeded-fix branch from 07b9bcf to 4434c2a Compare November 28, 2024 16:09

ti-chi-bot 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 Nov 28, 2024

fix tests

0494095

winoros force-pushed the loadneeded-fix branch from 4434c2a to 0494095 Compare November 28, 2024 16:24

winoros changed the title ~~statistics: try to eliminate the unnecessary log for sync load | tidb-test=pr/2435~~ statistics: try to eliminate the unnecessary log for sync load Nov 28, 2024

winoros added 2 commits November 29, 2024 03:36

make it more compatible than before

c1aa991

undo unnecessary test changes

f9f7ac1

winoros force-pushed the loadneeded-fix branch from fca25f1 to f9f7ac1 Compare November 28, 2024 19:36

winoros changed the title ~~statistics: try to eliminate the unnecessary log for sync load~~ statistics: correct some behavior of non-lite InitStats and stats sync load Nov 28, 2024

winoros changed the title ~~statistics: correct some behavior of non-lite InitStats and stats sync load~~ statistics: correct behavior of non-lite InitStats and stats sync load of no stats column Nov 28, 2024

winoros added 3 commits November 29, 2024 03:56

remove unused arg

e7a78b7

reduce changed lines

07cf605

add comments

b09bbbc

winoros added 3 commits November 29, 2024 04:30

unify the SQL gen

afe0211

undo unnecessary changes(will do it in future pr)

ac3ce70

correct the test once more after undo some main changes

ccd08ee

winoros commented Nov 28, 2024

View reviewed changes

winoros added 2 commits November 29, 2024 05:32

Merge remote-tracking branch 'origin/master' into loadneeded-fix

24666c1

fix test

811c04b

winoros commented Nov 28, 2024

View reviewed changes

hawkingrei approved these changes Nov 29, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 29, 2024

add one test

ccceaa1

Rustin170506 reviewed Dec 2, 2024

View reviewed changes

pkg/statistics/table.go Outdated Show resolved Hide resolved

winoros and others added 4 commits December 2, 2024 19:19

Update pkg/statistics/table.go

efc7643

Co-authored-by: Rustin <tech@rustin.me>

add some comments

9d940ad

Merge remote-tracking branch 'yiding/loadneeded-fix' into loadneeded-fix

1fdc4f3

fix bazel_prepare and fix typo

6d003ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

statistics: correct behavior of non-lite InitStats and stats sync load of no stats column #57803

statistics: correct behavior of non-lite InitStats and stats sync load of no stats column #57803

statistics: correct behavior of non-lite InitStats and stats sync load of no stats column #57803

Are you sure you want to change the base?

statistics: correct behavior of non-lite InitStats and stats sync load of no stats column #57803

Conversation

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[LGTM Timeline notifier]

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment