statistics: reduce allocation of types.Context by winoros · Pull Request #52615 · pingcap/tidb

winoros · 2024-04-15T12:16:21Z

What problem does this PR solve?

Issue Number: close #52342

Problem Summary:

We still have #52429, but we can reduce the allocations first.

What changed and how does it work?

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-04-15T12:51:38Z

Codecov Report

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

Project coverage is 55.6253%. Comparing base (40c81c0) to head (3a4f760).

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #52615         +/-   ##
=================================================
- Coverage   72.3860%   55.6253%   -16.7607%     
=================================================
  Files          1482       1593        +111     
  Lines        428754     598394     +169640     
=================================================
+ Hits         310358     332859      +22501     
- Misses        99088     242580     +143492     
- Partials      19308      22955       +3647

Flag	Coverage Δ
integration	`36.9334% <77.7777%> (?)`

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`49.8961% <ø> (+8.7287%)`	⬆️

0xPoe · 2024-04-18T09:03:40Z

/cc

0xPoe

Do you have any test results? Could you please put it on GitHub?

0xPoe · 2024-04-18T09:14:44Z

+		// Invalid date values may be inserted into table under some relaxed sql mode. Those values may exist in statistics.
+		// Hence, when reading statistics, we should skip invalid date check. See #39336.
+		sc := ctx.GetSessionVars().StmtCtx
+		sc.SetTypeFlags(sc.TypeFlags().WithIgnoreInvalidDateErr(true).WithIgnoreZeroInDate(true))


Could we use a function for it? Then we don't need to copy the comments and code again in other places.

Do you have any good ideas?
I'm not sure how to make it better(

0xPoe · 2024-04-18T09:15:55Z

 	prevSQL := s.sessionVars.StmtCtx.OriginalSQL
 	prevStmtType := s.sessionVars.StmtCtx.StmtType
 	prevTables := s.sessionVars.StmtCtx.Tables
+	prevTypeFlags := s.sessionVars.StmtCtx.TypeFlags()


Why do we need this change? Could you explain a little bit?

The useCurrentSession will return a func as the clean func after the session executes a SQL.
We need to make sure that the CLEAN operation will not clean the flag we set.

0xPoe · 2024-04-18T09:20:49Z

 	return prefixLens, nil
 }

-// ExtractTopN extracts topn from histogram.


Good catch!

winoros · 2024-04-18T14:11:32Z

Do you have any test results? Could you please put it on GitHub?

The test env is under rebuilding(

This reverts commit d30ac27.

This reverts commit 59d38e6.

This reverts commit 917ae39.

This reverts commit 93cf601.

This reverts commit 39caa08.

This reverts commit d97bdad.

time-and-fate

Please note that not all places use flags with FlagIgnoreInvalidDateErr and FlagIgnoreZeroInDateErr.
Others LGTM.

ti-chi-bot · 2024-04-28T19:00:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hawkingrei, time-and-fate

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/statistics/OWNERS~~ [hawkingrei,time-and-fate]

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-04-28T19:00:21Z

[LGTM Timeline notifier]

Timeline:

2024-04-15 16:38:41.674694488 +0000 UTC m=+269740.785740938: ☑️ agreed by hawkingrei.
2024-04-28 19:00:20.437909212 +0000 UTC m=+210974.195044785: ☑️ agreed by time-and-fate.

winoros · 2024-04-28T19:05:38Z

/retest

winoros · 2024-04-28T19:42:49Z

/cherrypick release-8.1

ti-chi-bot · 2024-04-28T19:43:31Z

@winoros: new pull request created to branch release-8.1: #52963.

Details

In response to this:

/cherrypick release-8.1

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.

close #52342

ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Apr 15, 2024

winoros force-pushed the reduce-alloc-typectx branch 2 times, most recently from 293bc35 to c648886 Compare April 15, 2024 13:51

hawkingrei reviewed Apr 15, 2024

View reviewed changes

Comment thread pkg/statistics/handle/bootstrap.go Outdated

hawkingrei reviewed Apr 15, 2024

View reviewed changes

Comment thread pkg/statistics/handle/handle.go Outdated

winoros force-pushed the reduce-alloc-typectx branch from c648886 to 6ada327 Compare April 15, 2024 13:59

hawkingrei approved these changes Apr 15, 2024

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Apr 15, 2024

ti-chi-bot Bot requested a review from 0xPoe April 18, 2024 09:03

0xPoe reviewed Apr 18, 2024

View reviewed changes

winoros added 13 commits April 28, 2024 23:41

statistics: reduce allocation of types.Context

d97bdad

relax the time check for related sctx

39caa08

statistics: reduce the unnecessary allocations of context

93cf601

fix lint and add intest check

917ae39

fix unexpected reset

59d38e6

move a closer place

d30ac27

Revert "move a closer place"

8b28834

This reverts commit d30ac27.

Revert "fix unexpected reset"

7ff2f95

This reverts commit 59d38e6.

Revert "fix lint and add intest check"

2157a82

This reverts commit 917ae39.

Revert "statistics: reduce the unnecessary allocations of context"

0068921

This reverts commit 93cf601.

Revert "relax the time check for related sctx"

673dcb5

This reverts commit 39caa08.

Revert "statistics: reduce allocation of types.Context"

60e510d

This reverts commit d97bdad.

use another way

b6faed1

winoros force-pushed the reduce-alloc-typectx branch from 7c679bd to b6faed1 Compare April 28, 2024 15:55

ti-chi-bot Bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2024

ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved labels Apr 28, 2024

fix bazel_prepare

3a4f760

winoros force-pushed the reduce-alloc-typectx branch from 89487df to 3a4f760 Compare April 28, 2024 16:38

time-and-fate approved these changes Apr 28, 2024

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 28, 2024

ti-chi-bot Bot merged commit 1d698c7 into pingcap:master Apr 28, 2024

winoros deleted the reduce-alloc-typectx branch April 28, 2024 19:42

ti-chi-bot mentioned this pull request Apr 28, 2024

statistics: reduce allocation of types.Context (#52615) #52963

Merged

1 of 13 tasks

ti-chi-bot Bot pushed a commit that referenced this pull request Apr 29, 2024

statistics: reduce allocation of types.Context (#52615) (#52963)

f37e5f3

close #52342

winoros mentioned this pull request Oct 8, 2024

tidb throws error when querying zero datetime with table statistics #56480

Closed

developer-Bushido mentioned this pull request May 20, 2026

Empty-partition placeholder histograms still trigger terror.Log via (*Time).Sub during plan-time cardinality estimation in v8.5.6 (#56480 follow-up) #68527

Closed

winoros mentioned this pull request Jun 24, 2026

statistics: zero TIMESTAMP bounds log terror errors during plan-time cardinality estimation #69440

Open

Conversation

winoros commented Apr 15, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

codecov Bot commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

0xPoe commented Apr 18, 2024

Uh oh!

0xPoe left a comment

Choose a reason for hiding this comment

Uh oh!

0xPoe Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

winoros Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

0xPoe Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

winoros Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

0xPoe Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

winoros commented Apr 18, 2024

Uh oh!

time-and-fate left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot Bot commented Apr 28, 2024

Uh oh!

ti-chi-bot Bot commented Apr 28, 2024

[LGTM Timeline notifier]

Uh oh!

winoros commented Apr 28, 2024

Uh oh!

winoros commented Apr 28, 2024

Uh oh!

ti-chi-bot commented Apr 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Apr 15, 2024 •

edited

Loading