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.

Sign up for GitHub

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

Jump to bottom

planner: fix wrong PointGet / TableDual plan reused in plan cache #23238

Merged
merged 4 commits into from
Mar 17, 2021
Merged

planner: fix wrong PointGet / TableDual plan reused in plan cache #23238

merged 4 commits into from
Mar 17, 2021

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Mar 10, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: close #23187
close #23144
close #23304
close #23290

Problem Summary:

Planner cannot find a plan for a query while it indeed can.

What is changed and how it works?

What's Changed:

  • if we prune paths in DeriveStats and only keep the TableDual / PointGet path, we mark this plan as not cacheable;
  • in skylinePruning we can enable the early return now if we find a path with empty range;

How it Works:

These plans have strong dependency on the param values, i.e, if the values change, those plans may lead to wrong query results.

Related changes

N/A

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • This may prevent some plans which have TableDual / PointGet from being cached, while those TableDual / PointGet may have no relationship with param values at all. For example, plan for query prepare stmt from 'select * from t1, t2 where t1.a > 9 and t1.a < 0 and t2.a > ? and t1.b = t2.b' cannot be cached then, but it is safe actually.

Release note

  • Fix wrong PointGet / TableDual plan reused in plan cache

@eurekaka eurekaka added type/bugfix This PR fixes a bug. sig/planner SIG: Planner epic/plan-cache labels Mar 10, 2021
@eurekaka eurekaka requested a review from a team as a code owner March 10, 2021 08:42
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 10, 2021
@eurekaka eurekaka requested a review from a team as a code owner March 10, 2021 08:56
eurekaka
eurekaka commented Mar 10, 2021
View reviewed changes
executor/explainfor_test.go
@@ -479,7 +479,4 @@ func (s *testPrepareSerialSuite) TestPointGetUserVarPlanCache(c *C) {
tk.MustQuery("execute stmt using @a").Check(testkit.Rows(
"2 4 2 2",
))
tk.MustQuery("select @@last_plan_from_cache").Check(testkit.Rows(
"1",
))
Copy link
Contributor Author

@eurekaka eurekaka Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sadly, this is the side effect of this PR, we may have false negative decision about "cache it or not", unless we track the whole range calculation procedure to know if any user variable is related to the final ranges. That would be trivial and easy to be broken by new code.

Copy link
Member

Choose a reason for hiding this comment

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

Can we change the test results instead of deleting these tests?
Because now we explicitly don't want to cache these plans to assure correctness.

@eurekaka
Copy link
Contributor Author

/run-check_dev_2

@eurekaka eurekaka removed request for a team March 10, 2021 09:04
@github-actions github-actions bot added the sig/execution SIG execution label Mar 10, 2021
@eurekaka
Copy link
Contributor Author

/run-all-tests

@ichn-hu ichn-hu mentioned this pull request Mar 10, 2021
Closed
time-and-fate
time-and-fate reviewed Mar 11, 2021
View reviewed changes
Copy link
Member

@time-and-fate time-and-fate left a comment

Choose a reason for hiding this comment

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

In (ds *DataSource).accessPathsForConds, which is for building partial paths for index merge, there're also logics like "if we have point get or empty range, remove other paths". Seems we should also set OptimDependOnMutableConst to true there?

planner/core/find_best_task.go Show resolved Hide resolved
@eurekaka eurekaka mentioned this pull request Mar 12, 2021
Closed
@ti-chi-bot 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 Mar 15, 2021
@eurekaka eurekaka mentioned this pull request Mar 16, 2021
Closed
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 17, 2021
time-and-fate
time-and-fate approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@time-and-fate time-and-fate left a comment

Choose a reason for hiding this comment

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

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • qw4990
  • time-and-fate

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 17, 2021
@eurekaka
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: b4466b7

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 17, 2021
@time-and-fate
Copy link
Member

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit 1ac53c5 into pingcap:master Mar 17, 2021
@eurekaka eurekaka deleted the path_prune branch March 17, 2021 09:01
@blacktear23
Copy link
Contributor

@eurekaka do we need cherry-pick to release branches?

@eurekaka eurekaka mentioned this pull request Mar 24, 2021
Closed
SabaPing pushed a commit to SabaPing/tidb that referenced this pull request Mar 25, 2021
@eurekaka @SabaPing
planner: fix wrong PointGet / TableDual plan reused in plan cache (pi…
f65520d 
…ngcap#23238)
@aytrack aytrack mentioned this pull request Mar 30, 2021
Closed
@ti-srebot ti-srebot mentioned this pull request Apr 15, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #24043

eurekaka added a commit to ti-srebot/tidb that referenced this pull request Apr 15, 2021
@eurekaka
cherry pick pingcap#23238 to release-5.0
bce34f6 
Signed-off-by: ti-srebot <[email protected]>
ti-chi-bot pushed a commit that referenced this pull request Apr 16, 2021
@ti-srebot
planner: fix wrong PointGet / TableDual plan reused in plan cache (#2…
4c49754 
…3238) (#24043)
@Reminiscent Reminiscent mentioned this pull request Aug 31, 2021
Closed
This was referenced Oct 8, 2021
Closed
Open
@Reminiscent Reminiscent mentioned this pull request Oct 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 approved these changes

@time-and-fate time-and-fate time-and-fate approved these changes

@winoros winoros Awaiting requested review from winoros

Assignees
No one assigned
Labels
epic/plan-cache needs-cherry-pick-release-5.0 sig/execution SIG execution sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
6 participants
@eurekaka @ti-chi-bot @time-and-fate @blacktear23 @ti-srebot @qw4990