planner: remove some risky cache operations in the plan builder (#23354) #23536

ti-srebot · 2021-03-25T07:07:25Z

cherry-pick #23354 to release-5.0
You can switch your code base to this Pull Request by using git-extras:

# In tidb repo:
git pr https://github.com/pingcap/tidb/pull/23536

After apply modifications, you can push your change to this PR via:

git push [email protected]:ti-srebot/tidb.git pr/23536:release-5.0-28c3748496b6

What problem does this PR solve?

Issue Number: close #22850

Problem Summary: planner: remove some risky cache operations in the plan builder

What is changed and how it works?

#21431 allows the plan builder to cache some logical plans to avoid unnecessary evaluation(in PlanBuilder.buildTableRefs), but this operation brings some risks.
Because the cached logical plans may be modified in some other places, and once they are modified, some implicit assumptions may be broken.

In this case(#22850):

the expression rewriter has an implicit assumption that len(p.Schema().Cols) == len(p.OutputNames()) (in getExpressionRewriter).
the subquery in the SQL is handled twice during processing(in buildProjection): the first happens when pre-processing user variables(preprocessUserVarTypes) and the second happens when rewriting selection fields(rewriteWithPreprocess);
during the second processing, the result cached in first is returned, but the cached logical plan has been modified since its output names have been pruned, so the implicit assumption required by the rewriter is broken.

Check List

Tests

Unit test

Release note

planner: remove some risky cache operations in the plan builder

Signed-off-by: ti-srebot <[email protected]>

ti-srebot · 2021-03-25T07:07:28Z

/run-all-tests

ti-srebot · 2021-03-25T07:07:36Z

@qw4990 you're already a collaborator in bot's repo.

zimulala · 2021-03-25T07:09:50Z

/cc @winoros

zimulala · 2021-03-25T07:33:33Z

/cc @Reminiscent @eurekaka

zimulala · 2021-03-25T07:34:24Z

/cc @rebelice

Reminiscent

LGTM

ti-chi-bot · 2021-03-25T07:48:13Z

@Reminiscent: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 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.

ti-chi-bot · 2021-03-25T07:48:15Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Reminiscent
winoros

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 · 2021-03-25T07:48:35Z

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

Commit hash: 8c0ab9c

qw4990 · 2021-03-25T08:06:05Z

/run-all-tests

qw4990 · 2021-03-25T13:32:08Z

/run-all-tests

qw4990 · 2021-03-25T13:33:02Z

/merge

ti-chi-bot · 2021-03-25T13:33:05Z

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

Commit hash: 5c421482c417b23a009d8c9aa21c8e3e706a976c

qw4990 · 2021-03-25T13:33:58Z

/merge

qw4990 · 2021-03-25T13:34:06Z

/merge

zimulala · 2021-03-26T02:25:23Z

/merge

ti-chi-bot · 2021-03-26T02:25:25Z

@zimulala: /merge is only allowed for the committers in list.

In response to this:

/merge

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.

zimulala · 2021-03-26T02:45:35Z

/cc @qw4990

zimulala · 2021-03-26T02:49:30Z

/run-all-tests

XuHuaiyu · 2021-03-26T02:50:54Z

/merge

qw4990 · 2021-03-26T03:41:44Z

/merge

ti-chi-bot · 2021-03-26T03:41:47Z

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

Commit hash: 64bcaeb

ti-chi-bot · 2021-03-26T05:25:35Z

@ti-srebot: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

cherry pick pingcap#23354 to release-5.0

0902692

Signed-off-by: ti-srebot <[email protected]>

ti-srebot requested review from a team as code owners March 25, 2021 07:07

ti-srebot requested review from lzmhhh123 and removed request for a team March 25, 2021 07:07

ti-srebot mentioned this pull request Mar 25, 2021

planner: remove some risky cache operations in the plan builder #23354

Merged

ti-srebot added sig/execution SIG execution sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/5.0-cherry-pick type/bugfix This PR fixes a bug. labels Mar 25, 2021

ti-srebot requested review from rebelice and winoros March 25, 2021 07:07

ti-srebot added this to the v5.0.0 ga milestone Mar 25, 2021

ti-srebot assigned qw4990 Mar 25, 2021

winoros approved these changes Mar 25, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 25, 2021

fix CI

b438e5e

ti-chi-bot requested review from eurekaka and Reminiscent March 25, 2021 07:33

fix ci

8c0ab9c

Reminiscent approved these changes Mar 25, 2021

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 25, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021

zimulala added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Mar 25, 2021

fixup

82dd0c4

qw4990 force-pushed the release-5.0-28c3748496b6 branch from c0157ef to 82dd0c4 Compare March 25, 2021 13:31

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 25, 2021

ti-chi-bot requested a review from qw4990 March 26, 2021 02:45

fix ci

2329c9a

qw4990 force-pushed the release-5.0-28c3748496b6 branch from a862daa to 2329c9a Compare March 26, 2021 03:40

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 26, 2021

Merge branch 'release-5.0' into release-5.0-28c3748496b6

64bcaeb

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 26, 2021

ti-chi-bot added 2 commits March 26, 2021 13:15

Merge branch 'release-5.0' into release-5.0-28c3748496b6

e6016b3

Merge branch 'release-5.0' into release-5.0-28c3748496b6

62a5ccc

ti-chi-bot merged commit 7b0a996 into pingcap:release-5.0 Mar 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: remove some risky cache operations in the plan builder (#23354) #23536

planner: remove some risky cache operations in the plan builder (#23354) #23536

ti-srebot commented Mar 25, 2021 •

edited

Loading

ti-srebot commented Mar 25, 2021

ti-srebot commented Mar 25, 2021

zimulala commented Mar 25, 2021

zimulala commented Mar 25, 2021

zimulala commented Mar 25, 2021

Reminiscent left a comment

ti-chi-bot commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

zimulala commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

zimulala commented Mar 26, 2021

zimulala commented Mar 26, 2021

XuHuaiyu commented Mar 26, 2021

qw4990 commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

planner: remove some risky cache operations in the plan builder (#23354) #23536

planner: remove some risky cache operations in the plan builder (#23354) #23536

Conversation

ti-srebot commented Mar 25, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-srebot commented Mar 25, 2021

ti-srebot commented Mar 25, 2021

zimulala commented Mar 25, 2021

zimulala commented Mar 25, 2021

zimulala commented Mar 25, 2021

Reminiscent left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

ti-chi-bot commented Mar 25, 2021

qw4990 commented Mar 25, 2021

qw4990 commented Mar 25, 2021

zimulala commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

zimulala commented Mar 26, 2021

zimulala commented Mar 26, 2021

XuHuaiyu commented Mar 26, 2021

qw4990 commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

ti-srebot commented Mar 25, 2021 •

edited

Loading