variable: unifiy MaxExecuteTime usage and fix some problem #50915

jiyfhust · 2024-02-01T13:00:37Z

What problem does this PR solve?

Please create an issue first to describe the problem.

There MUST be one line starting with "Issue Number: " and
linking the relevant issues via the "close" or "ref".

For more info, check https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/contribute-code.html#referring-to-an-issue.

-->

Issue Number: close #50914, close #50872

Problem Summary:

What changed and how does it work?

provide a Function GetMaxExecutionTime for SessionVars.
Fix two potential bug, caused by not properly used MaxExecutionTime, one about plan_stats，the other expression/build_in.
Fix a bug related with tidb_stats_load_sync_wait
unify code for GetMaxExecutionTime.

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

tiprow · 2024-02-01T13:00:51Z

Hi @jiyfhust. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

jiyfhust · 2024-02-01T13:01:05Z

/ok-to-test

codecov · 2024-02-01T13:14:56Z

Codecov Report

Merging #50915 (9a34a94) into master (1f03420) will increase coverage by 2.1090%.
Report is 121 commits behind head on master.
The diff coverage is 77.7777%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #50915        +/-   ##
================================================
+ Coverage   70.5752%   72.6843%   +2.1090%     
================================================
  Files          1467       1467                
  Lines        434756     434772        +16     
================================================
+ Hits         306830     316011      +9181     
+ Misses       108651      98830      -9821     
- Partials      19275      19931       +656

Flag	Coverage Δ
integration	`49.2220% <66.6666%> (?)`
unit	`70.3157% <77.7777%> (-0.0529%)`	⬇️

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

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

jiyfhust · 2024-02-04T01:26:21Z

@YangKeao Could you take a look at this? It is more like a code unifiy enhencement.

hawkingrei · 2024-02-04T09:13:00Z

/retest

winoros · 2024-02-05T12:52:58Z

pkg/planner/core/plan_stats.go

@@ -38,7 +38,7 @@ func (collectPredicateColumnsPoint) optimize(_ context.Context, plan LogicalPlan
 		return plan, planChanged, nil
 	}
 	predicateNeeded := variable.EnableColumnTracking.Load()
-	syncWait := plan.SCtx().GetSessionVars().StatsLoadSyncWait * time.Millisecond.Nanoseconds()


This change is not about the max execution time, just wrong time unit is used?

Yes, it fix issue #50872.

jiyfhust · 2024-02-06T03:24:01Z

/test pull-integration-ddl-test

tiprow · 2024-02-06T03:24:22Z

@jiyfhust: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tidb_parser_test
/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test pull-integration-ddl-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.

ti-chi-bot · 2024-02-13T20:17:12Z

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.

jiyfhust · 2024-02-19T06:55:16Z

PTAL @winoros @hawkingrei @qw4990

jiyfhust · 2024-02-26T08:51:45Z

brothers, look look.

jiyfhust · 2024-02-27T09:12:07Z

/check-issue-triage-complete

jiyfhust · 2024-03-04T01:12:45Z

@qw4990
PTAL PTAL PTAL

ti-chi-bot · 2024-03-04T08:10:16Z

[LGTM Timeline notifier]

Timeline:

2024-02-28 16:09:27.394039452 +0000 UTC m=+1064656.141662556: ☑️ agreed by winoros.
2024-03-04 08:10:14.870912869 +0000 UTC m=+68241.893159259: ☑️ agreed by qw4990.

jiyfhust · 2024-03-04T08:28:44Z

please assign yudongusa, zanmato1984 for approval

Can you take a look at this, thanks. @zanmato1984

windtalker

LGTM

easonn7 · 2024-03-04T09:49:03Z

/approve

ti-chi-bot · 2024-03-04T09:49:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: easonn7, qw4990, windtalker, winoros

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:

~~OWNERS~~ [qw4990,windtalker,winoros]
~~pkg/expression/OWNERS~~ [windtalker]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]

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

winoros · 2024-04-15T13:35:05Z

/run-cherry-picker release-7.1
/run-cherry-picker release-7.5

winoros · 2024-04-15T13:43:01Z

/cherrypick release-7.1
/cherrypick release-7.5

ti-chi-bot · 2024-04-15T13:43:45Z

@winoros: new pull request created to branch release-7.1: #52618.

In response to this:

/cherrypick release-7.1
/cherrypick release-7.5

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.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2024-04-15T13:44:27Z

@winoros: new pull request created to branch release-7.5: #52619.

In response to this:

/cherrypick release-7.1
/cherrypick release-7.5

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.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

…52619) close #50872, close #50914

This reverts commit d746169.

…52618) close #50872, close #50914

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 1, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Feb 1, 2024

jiyfhust mentioned this pull request Feb 1, 2024

tidb_stats_load_sync_wait Parameter does not take effect #50872

Closed

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Feb 4, 2024

ti-chi-bot bot added do-not-merge/needs-tests-checked and removed do-not-merge/needs-tests-checked labels Feb 4, 2024

hawkingrei removed the do-not-merge/needs-triage-completed label Feb 4, 2024

hawkingrei self-requested a review February 4, 2024 10:02

ti-chi-bot bot added the do-not-merge/needs-triage-completed label Feb 5, 2024

winoros reviewed Feb 5, 2024

View reviewed changes

qw4990 self-requested a review February 8, 2024 02:28

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

unifiy MaxExecuteTime usage

9a34a94

jiyfhust force-pushed the unified_max_execute_time branch 2 times, most recently from bd5fcb3 to 9a34a94 Compare February 19, 2024 07:45

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

hawkingrei removed the do-not-merge/needs-triage-completed label Feb 28, 2024

winoros approved these changes Feb 28, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Feb 28, 2024

qw4990 approved these changes Mar 4, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 4, 2024

windtalker approved these changes Mar 4, 2024

View reviewed changes

ti-chi-bot bot added the approved label Mar 4, 2024

ti-chi-bot bot merged commit 13bff87 into pingcap:master Mar 4, 2024
23 checks passed

jiyfhust deleted the unified_max_execute_time branch March 4, 2024 12:34

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 15, 2024

This is an automated cherry-pick of pingcap#50915

d746169

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

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

variable: unifiy MaxExecuteTime usage and fix some problem (#50915) #52618

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Apr 15, 2024

This is an automated cherry-pick of pingcap#50915

22e77db

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

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

variable: unifiy MaxExecuteTime usage and fix some problem (#50915) #52619

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request May 28, 2024

variable: unifiy MaxExecuteTime usage and fix some problem (#50915) (#…

62e0c8d

…52619) close #50872, close #50914

winoros added a commit to ti-chi-bot/tidb that referenced this pull request Nov 11, 2024

Revert "This is an automated cherry-pick of pingcap#50915"

e011549

This reverts commit d746169.

winoros added a commit to ti-chi-bot/tidb that referenced this pull request Nov 11, 2024

cherry-pick pingcap#50915

8696741

ti-chi-bot bot pushed a commit that referenced this pull request Nov 11, 2024

variable: unifiy MaxExecuteTime usage and fix some problem (#50915) (#…

c6f28f0

…52618) close #50872, close #50914

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

variable: unifiy MaxExecuteTime usage and fix some problem #50915

variable: unifiy MaxExecuteTime usage and fix some problem #50915

jiyfhust commented Feb 1, 2024 •

edited

Loading

tiprow bot commented Feb 1, 2024

jiyfhust commented Feb 1, 2024

codecov bot commented Feb 1, 2024 •

edited

Loading

jiyfhust commented Feb 4, 2024 •

edited

Loading

hawkingrei commented Feb 4, 2024

winoros Feb 5, 2024

jiyfhust Feb 6, 2024

jiyfhust commented Feb 6, 2024

tiprow bot commented Feb 6, 2024

ti-chi-bot bot commented Feb 13, 2024

jiyfhust commented Feb 19, 2024

jiyfhust commented Feb 26, 2024

jiyfhust commented Feb 27, 2024

jiyfhust commented Mar 4, 2024

ti-chi-bot bot commented Mar 4, 2024

jiyfhust commented Mar 4, 2024

windtalker left a comment

easonn7 commented Mar 4, 2024

ti-chi-bot bot commented Mar 4, 2024

winoros commented Apr 15, 2024

winoros commented Apr 15, 2024

ti-chi-bot commented Apr 15, 2024

ti-chi-bot commented Apr 15, 2024

variable: unifiy MaxExecuteTime usage and fix some problem #50915

variable: unifiy MaxExecuteTime usage and fix some problem #50915

Conversation

jiyfhust commented Feb 1, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Feb 1, 2024

jiyfhust commented Feb 1, 2024

codecov bot commented Feb 1, 2024 • edited Loading

Codecov Report

jiyfhust commented Feb 4, 2024 • edited Loading

hawkingrei commented Feb 4, 2024

winoros Feb 5, 2024

Choose a reason for hiding this comment

jiyfhust Feb 6, 2024

Choose a reason for hiding this comment

jiyfhust commented Feb 6, 2024

tiprow bot commented Feb 6, 2024

ti-chi-bot bot commented Feb 13, 2024

jiyfhust commented Feb 19, 2024

jiyfhust commented Feb 26, 2024

jiyfhust commented Feb 27, 2024

jiyfhust commented Mar 4, 2024

ti-chi-bot bot commented Mar 4, 2024

[LGTM Timeline notifier]

jiyfhust commented Mar 4, 2024

windtalker left a comment

Choose a reason for hiding this comment

easonn7 commented Mar 4, 2024

ti-chi-bot bot commented Mar 4, 2024

winoros commented Apr 15, 2024

winoros commented Apr 15, 2024

ti-chi-bot commented Apr 15, 2024

ti-chi-bot commented Apr 15, 2024

jiyfhust commented Feb 1, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

jiyfhust commented Feb 4, 2024 •

edited

Loading