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: not push down mpp/join to TiFlash in some cases that TiFlash not supported the query #23259

Merged
merged 18 commits into from
Mar 12, 2021
Merged

planner: not push down mpp/join to TiFlash in some cases that TiFlash not supported the query #23259

merged 18 commits into from
Mar 12, 2021

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented Mar 11, 2021 •
edited
Loading

What problem does this PR solve?

In some cases, TiFlash does not support MPP execution, so TiDB should disable generating mpp plan.
Issue Number: close pingcap/tiflash#1520

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

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

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • Disable mpp execution if the new collation is on.

@windtalker windtalker requested review from a team as code owners March 11, 2021 08:02
@windtalker windtalker requested review from wshwsh12 and XuHuaiyu and removed request for a team March 11, 2021 08:02
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 11, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Mar 11, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

  • *: what's changed

@windtalker windtalker changed the title Not pushdown to tiflash in some cases planner: not push down mpp/join to TiFlash in some cases that TiFlash not supported the query Mar 11, 2021
@hanfei1991
Copy link
Member

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 11, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Mar 11, 2021
@ichn-hu
Copy link
Contributor

ichn-hu commented Mar 11, 2021

/run-all-tests

fzhedu
fzhedu reviewed Mar 12, 2021
View reviewed changes
planner/core/integration_test.go
tk.MustExec("analyze table table_1")

// Create virtual tiflash replica info.
dom := domain.GetDomain(tk.Se)
Copy link
Contributor

Choose a reason for hiding this comment

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

would here set tiflash replica like this way?
https://github.com/pingcap/tidb/pull/23056/files#diff-71a2dde4dc9f29476f96c621ad0c11f6fc3f7b45185e11cb16f44acf46954cfeR328

fzhedu
fzhedu reviewed Mar 12, 2021
View reviewed changes
planner/core/exhaust_physical_plans.go
@@ -1712,6 +1712,29 @@ func (p *LogicalJoin) exhaustPhysicalPlans(prop *property.PhysicalProperty) ([]P
return joins, true
}

func canExprsInJoinPushdown(p *LogicalJoin, storeType kv.StoreType) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there similar case for agg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No

@fzhedu
Copy link
Contributor

fzhedu commented Mar 12, 2021

/LGTM

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • fzhedu
  • hanfei1991

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 12, 2021
@fzhedu
Copy link
Contributor

fzhedu commented Mar 12, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: c661abe

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021
@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker
Copy link
Contributor Author

/run-check_dev_2

@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker
Copy link
Contributor Author

/run-check_dev_2

@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker
Revert "address comments"
ab4daa7 
This reverts commit c661abe.
@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021
@windtalker
Copy link
Contributor Author

/run-all-tests

@fzhedu
Copy link
Contributor

fzhedu commented Mar 12, 2021

/lgtm

@fzhedu
Copy link
Contributor

fzhedu commented Mar 12, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: ab4daa7

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

@windtalker: 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.

@ti-chi-bot ti-chi-bot merged commit cd199b2 into pingcap:master Mar 12, 2021
@windtalker windtalker deleted the not_pushdown_to_tiflash_in_some_cases branch March 12, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzhedu fzhedu fzhedu left review comments

@wshwsh12 wshwsh12 Awaiting requested review from wshwsh12

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees
No one assigned
Labels
sig/execution SIG execution 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TiDB throw "runtime error: invalid memory address or nil pointer dereference" error when running mpp sql
6 participants
@windtalker @sre-bot @hanfei1991 @ichn-hu @fzhedu @ti-chi-bot