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,executor: enable plan cache for partition table #19124

Merged
merged 12 commits into from
Sep 8, 2020

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Aug 11, 2020

What problem does this PR solve?

Problem Summary:

In the new implementation, queries on the partition table are not converted to UnionScan and partition pruning on in executor builder.
So it is possible to use plan cache for the partition table now.
This PR enables plan cache for the partition table.

What is changed and how it works?

What's Changed:

How it Works:

Revert part of the #16375

Check List

Tests

  • Unit test

The old unit test TestPrepareCacheForPartition should cover the changes.

Release note

  • Enable prepared plan cache for the partitioned table.

@tiancaiamao tiancaiamao requested review from a team as code owners August 11, 2020 05:50
@tiancaiamao tiancaiamao requested review from SunRunAway and removed request for a team August 11, 2020 05:50
@tiancaiamao
Copy link
Contributor Author

PTAL @imtbkcat @lysu

@codecov
Copy link

codecov bot commented Aug 11, 2020

Codecov Report

Merging #19124 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #19124   +/-   ##
===========================================
  Coverage   79.1163%   79.1163%           
===========================================
  Files           547        547           
  Lines        148772     148772           
===========================================
  Hits         117703     117703           
  Misses        21606      21606           
  Partials       9463       9463           

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

@eurekaka PTAL

@zz-jason zz-jason requested a review from eurekaka September 1, 2020 05:07
@imtbkcat
Copy link

imtbkcat commented Sep 1, 2020

MySQL [test]> explain select * from t where a = 9;
+------------------------+---------+-----------+---------------+---------------------------------------------+
| id                     | estRows | task      | access object | operator info                               |
+------------------------+---------+-----------+---------------+---------------------------------------------+
| TableReader_6          | 1.00    | root      | partition:p0  | data:TableRangeScan_5                       |
| └─TableRangeScan_5     | 1.00    | cop[tikv] | table:t       | range:[9,9], keep order:false, stats:pseudo |
+------------------------+---------+-----------+---------------+---------------------------------------------+
2 rows in set (0.001 sec)

MySQL [test]> EXECUTE st1 USING @a;
+---+------+
| a | b    |
+---+------+
| 9 |    9 |
+---+------+
1 row in set (0.001 sec)

MySQL [test]> explain for connection 3;
+------------------------+---------+-----------+---------------+---------------------------------------------+
| id                     | estRows | task      | access object | operator info                               |
+------------------------+---------+-----------+---------------+---------------------------------------------+
| TableReader_6          | 1.00    | root      |               | data:TableRangeScan_5                       |
| └─TableRangeScan_5     | 1.00    | cop[tikv] | table:t       | range:[9,9], keep order:false, stats:pseudo |
+------------------------+---------+-----------+---------------+---------------------------------------------+
2 rows in set (0.000 sec)

it seems EXPLAIN can not show the partiton name after prepare/execute.

Copy link

@imtbkcat imtbkcat left a comment

@lysu lysu requested review from lysu and winoros September 1, 2020 06:12
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

Don't we need to add some new tests?

@ti-srebot
Copy link
Contributor

@sre-bot
Copy link
Contributor

sre-bot commented Sep 3, 2020

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 7, 2020
@tiancaiamao
Copy link
Contributor Author

PTAL @winoros

Copy link
Member

@winoros winoros 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 8, 2020
@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/rebuild

@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/merge

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

/run-all-tests

@ti-srebot
Copy link
Contributor

@tiancaiamao merge failed.

@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@tiancaiamao merge failed.

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@lysu
Copy link
Contributor

lysu commented Sep 8, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 19846

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@tiancaiamao merge failed.

@tiancaiamao
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 48388a7 into pingcap:master Sep 8, 2020
@tiancaiamao tiancaiamao deleted the plan-cache branch September 8, 2020 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/executor sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants