Skip to content

planner: let projection can push down to TiFlash when mpp is not enforced.#26207

Closed
LittleFall wants to merge 3 commits into
pingcap:masterfrom
LittleFall:bugfix/proj-enforce-pushdown
Closed

planner: let projection can push down to TiFlash when mpp is not enforced.#26207
LittleFall wants to merge 3 commits into
pingcap:masterfrom
LittleFall:bugfix/proj-enforce-pushdown

Conversation

@LittleFall
Copy link
Copy Markdown
Contributor

@LittleFall LittleFall commented Jul 13, 2021

What problem does this PR solve?

Problem Summary: #25450 support projection push down to TiFlash, but it only takes effect when mpp mode is enforced. This changes the semantics of tidb_enforce_mpp: It will introduce new optimization rules instead of just ignoring the optimizer cost estimate.

What is changed and how it works?

Proposal: xxx

What's Changed: make this optimization rule takes effect in the normal situation.

Check List

Tests

  • Integration test

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory

It may bring more optimizer search space and branches, but in TP business, it will be blocked by the following check p.canPushToCop.

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

  • Support projection push down to TiFlash in mpp mode to speed up queries.

@ti-chi-bot
Copy link
Copy Markdown
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 13, 2021
@LittleFall
Copy link
Copy Markdown
Contributor Author

/cc @hanfei1991 @rebelice

@LittleFall LittleFall force-pushed the bugfix/proj-enforce-pushdown branch from 6ddd78b to b8eecf1 Compare July 13, 2021 09:58
@ti-chi-bot ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 13, 2021
@LittleFall
Copy link
Copy Markdown
Contributor Author

/sig planner

@ti-chi-bot ti-chi-bot added the sig/planner SIG: Planner label Jul 13, 2021
@LittleFall LittleFall force-pushed the bugfix/proj-enforce-pushdown branch from 8ad0483 to 676225a Compare July 13, 2021 13:42
@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 13, 2021
@LittleFall LittleFall requested a review from lzmhhh123 July 13, 2021 13:46
Comment thread bindinfo/bind_test.go
tk.MustExec("drop table if exists t")
tk.MustExec("create table t(a int, b int, index idx_a(a), index idx_b(b))")
tk.MustExec("insert into t values (1,1), (2,2), (3,3), (4,4), (5,5), (6,6), (7,7), (8,8), (9,9), (10,10)")
tk.MustExec("create table t(a int, index idx_a(a))")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happened to these test?

Comment thread bindinfo/bind_test.go

// Even if index of TiKV has lower cost, it chooses TiFlash.
rows = tk.MustQuery("explain select * from t where a >= 11 and b >= 11").Rows()
rows = tk.MustQuery("explain select * from t where a >= 11").Rows()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change the binding tests?

@LittleFall
Copy link
Copy Markdown
Contributor Author

LittleFall commented Jul 14, 2021

after #25450 and this pr is patched, the cost of some plan will be changed (The cost of plan on tiflash is reduced because it can use mpp), �so some test is broken.

@LittleFall
Copy link
Copy Markdown
Contributor Author

after #25450 and this pr is patched, the cost of some plan will be changed (The cost of plan on tiflash is reduced), �so some test is broken.

For example, the bind test. In this test, it is assumed that it will select tikv. In fact, it selected tiflash. In order to ensure that it reaches tikv, I modified the test sql.
@lzmhhh123 @hanfei1991

@LittleFall
Copy link
Copy Markdown
Contributor Author

another things is after this patch, the estCost in explain format='verbose' seems not accurate.

drop table t;
create table t(a int, index idx(a));
alter table t set tiflash replica 1;
analyze table t;
explain format='verbose' select /*+ read_from_storage(tikv[t]) */ * from t where a>=11; -- 11473
explain format='verbose' select * from t where a>=11;         -- 12098
explain format='verbose' select /*+ read_from_storage(tiflash[t]) */ * from t where a>=11; -- 12098

@LittleFall
Copy link
Copy Markdown
Contributor Author

I will close this pr because the influence on optimizer cost model can't be evaluated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/planner SIG: Planner size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants