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.

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

planner: do not cache prepared plan if optimization depends on mutable constant #22349

Merged
merged 4 commits into from
Jan 12, 2021

Conversation

xuyifangreeneyes
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #22167

Problem Summary:
Incorrectly reuse cached plan which turns outer join to inner join.

What is changed and how it works?

What's Changed & How it Works:
EvaluateExprWithNull is used in several optimizations such as converting outer join to inner join. If the result of EvaluateExprWithNull depends on parameters of execute statement, whether the optimizations can be used is affected by the parameters. When optimizing a prepared plan, if expr passed into EvaluateExprWithNull contains mutable constant, we still do optimization but do not cache the plan.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests a

  • Unit test
  • Integration test

Release note

  • fix a bug about incorrectly reuse cached plan

@xuyifangreeneyes xuyifangreeneyes requested review from a team as code owners January 11, 2021 11:07
@xuyifangreeneyes xuyifangreeneyes requested review from XuHuaiyu and removed request for a team January 11, 2021 11:07
@xuyifangreeneyes
Copy link
Contributor Author

/run-all-tests

@qw4990 qw4990 requested review from qw4990 and eurekaka and removed request for XuHuaiyu January 11, 2021 11:24
@qw4990 qw4990 added needs-cherry-pick-4.0 sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Jan 11, 2021
@@ -374,7 +374,7 @@ REBUILD:
e.names = names
e.Plan = p
_, isTableDual := p.(*PhysicalTableDual)
if !isTableDual && prepared.UseCache {
if !isTableDual && prepared.UseCache && !stmtCtx.OptimDependOnMutableConst {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. It seems all places that use ContainMutableConst should be modified. We can only modify EvaluateExprWithNull in this PR and change all other places later in another PR.

Copy link
Contributor

@qw4990 qw4990 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 11, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 11, 2021

PTAL @eurekaka

@ichn-hu ichn-hu mentioned this pull request Jan 11, 2021
@xuyifangreeneyes
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@Reminiscent Reminiscent 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 12, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 12, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 12, 2021

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 12, 2021
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 22279

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit e3108df into pingcap:master Jan 12, 2021
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jan 12, 2021
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #22353

@ti-srebot
Copy link
Contributor

cherry pick to release-5.0-rc in PR #22354

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/planner SIG: Planner 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
Development

Successfully merging this pull request may close these issues.

incorrectly reuse cached plan which turns outer join to inner join
4 participants