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

plan: make query on partition table not cacheable #16375

Merged
merged 7 commits into from
Apr 22, 2020
Merged

plan: make query on partition table not cacheable #16375

merged 7 commits into from
Apr 22, 2020

Conversation

imtbkcat
Copy link

What problem does this PR solve?

Issue Number: close #16364

Problem Summary:
#15697 will check whether plan is a query on partition table and will pass by if is true. But there is still some problems bring by stmtCtx.UseCache. If a plan is Cacheable, stmtCtx.UseCache is set to true. So the simpleRewriter has different output compare with not cached plan.

For example, in #16364. If prepare-plan-cache is on, although query on partition table is not cached, it has different plan.

not use cache:
lt(test.employees.id, 3)
use cache:
lt(cast(test.employees.id), 3)

this small change in rewriter cause partition pruning fail.

What is changed and how it works?

Proposal: xxx

What's Changed:
add partition check in Cacheable function rather than getPhysicalPlan

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

  • No

Release note

@imtbkcat imtbkcat requested review from a team as code owners April 14, 2020 14:01
@ghost ghost removed request for a team April 14, 2020 14:01
@imtbkcat imtbkcat requested a review from jackysp April 14, 2020 14:02
@imtbkcat imtbkcat force-pushed the fixPartitionPlancache branch from b0be604 to e9b48cf Compare April 14, 2020 14:04
@github-actions github-actions bot added the sig/execution SIG execution label Apr 14, 2020
@codecov
Copy link

codecov bot commented Apr 14, 2020 •
edited
Loading

Codecov Report

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

@@             Coverage Diff             @@
##             master     #16375   +/-   ##
===========================================
  Coverage   80.3928%   80.3928%           
===========================================
  Files           506        506           
  Lines        137103     137103           
===========================================
  Hits         110221     110221           
  Misses        18285      18285           
  Partials       8597       8597

tiancaiamao
tiancaiamao reviewed Apr 15, 2020
View reviewed changes
Copy link
Contributor

@tiancaiamao tiancaiamao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Have you figure out why there is a cast and is that evitable？
If there is no better way, I think this one is an acceptable solution.

planner/core/cacheable_checker.go
}
return in, false
}

func (checker *cacheableChecker) isPartitionTable(tn *ast.TableName) bool {
tb, err := checker.schema.TableByName(tn.Schema, tn.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change may have some performance impact.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this PR incur significant performance regression? it just adds one more check for TableName.

Because of the check here. @eurekaka

Copy link
Contributor

@eurekaka eurekaka Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Are those 2 map lookups so expensive?

Copy link
Contributor

Choose a reason for hiding this comment

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

schema.TableByName() is O(N) operation.
And the cacheableChecker itself need to handle each tableName in the AST.

@tiancaiamao
Copy link
Contributor

/sysbench

@imtbkcat
Copy link
Author

@XuHuaiyu PTAL

@tiancaiamao
Copy link
Contributor

LGTM
/bench

@tiancaiamao
Copy link
Contributor

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 22664a706f093adcff82bffab6ca7367add314b7
+++ tidb: 14f8ccc72380267879103443e7743e665ba85c00
tikv: 7d13ca02374582bd7714d3298efb74909d69ca64
pd: 878ac7e830af195912603501d5d4de8ccffff232
================================================================================
oltp_update_index:
    * QPS: 4235.82 ± 4.34% (std=129.44) delta: 2.13% (p=0.384)
    * Latency p50: 30.24 ± 4.38% (std=0.92) delta: -2.06%
    * Latency p99: 55.11 ± 5.01% (std=1.91) delta: -4.83%
            
oltp_read_write:
    * QPS: 14106.59 ± 1.30% (std=128.84) delta: 0.82% (p=0.226)
    * Latency p50: 181.84 ± 1.35% (std=1.69) delta: -0.82%
    * Latency p99: 345.67 ± 2.24% (std=5.14) delta: -0.74%
            
oltp_point_select:
    * QPS: 39013.27 ± 0.42% (std=117.78) delta: -2.93% (p=0.221)
    * Latency p50: 3.28 ± 0.41% (std=0.01) delta: 2.24%
    * Latency p99: 11.14 ± 2.76% (std=0.23) delta: -0.87%
            
oltp_update_non_index:
    * QPS: 8854.25 ± 0.41% (std=25.87) delta: 0.06% (p=0.367)
    * Latency p50: 14.45 ± 0.42% (std=0.04) delta: -0.09%
    * Latency p99: 29.54 ± 1.20% (std=0.25) delta: -0.16%

@tiancaiamao
Copy link
Contributor

PTAL @zz-jason

@imtbkcat imtbkcat force-pushed the fixPartitionPlancache branch from 14f8ccc to 2a1bc90 Compare April 20, 2020 03:16
@imtbkcat
Copy link
Author

it's seems this pr bring performance impact that can not be ignore.

@tiancaiamao
Copy link
Contributor

What's your opinion? @XuHuaiyu

@tiancaiamao tiancaiamao modified the milestones: v4.0.0-ga, v4.0.0-rc.1 Apr 21, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

/run-all-tests

@eurekaka
Copy link
Contributor

Why would this PR incur significant performance regression? it just adds one more check for TableName.

@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

@imtbkcat merge failed.

@eurekaka
Copy link
Contributor

/run-all-tests

@imtbkcat
Copy link
Author

/run-common-test

@imtbkcat
Copy link
Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

/run-all-tests

@sre-bot sre-bot merged commit 79211fe into pingcap:master Apr 22, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 22, 2020
@sre-bot
plan: make query on partition table not cacheable (pingcap#16375)
a516039
@sre-bot sre-bot mentioned this pull request Apr 22, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Apr 22, 2020

cherry pick to release-4.0 in PR #16723

@eurekaka eurekaka mentioned this pull request Apr 23, 2020
Closed
@imtbkcat
Copy link
Author

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 23, 2020
@sre-bot
cherry pick pingcap#16375 to release-3.0
53057ad 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Apr 23, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

cherry pick to release-3.0 in PR #16759

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 23, 2020
@sre-bot
cherry pick pingcap#16375 to release-3.1
809c026 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Apr 23, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

cherry pick to release-3.1 in PR #16760

zz-jason pushed a commit that referenced this pull request Apr 23, 2020
@sre-bot
plan: make query on partition table not cacheable (#16375) (#16723)
94aff20
@crazycs520 crazycs520 modified the milestones: v4.0.0-rc.2, v4.0.0-rc.1 Apr 26, 2020
sre-bot added a commit that referenced this pull request Apr 29, 2020
@sre-bot
plan: make query on partition table not cacheable (#16375) (#16760)
c4de800
zz-jason pushed a commit that referenced this pull request Apr 29, 2020
@sre-bot
plan: make query on partition table not cacheable (#16375) (#16759)
447106a
@tiancaiamao tiancaiamao mentioned this pull request Aug 11, 2020
Merged
@tiancaiamao tiancaiamao mentioned this pull request Nov 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eurekaka eurekaka eurekaka approved these changes

@tiancaiamao tiancaiamao tiancaiamao left review comments

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

@jackysp jackysp Awaiting requested review from jackysp

Assignees
No one assigned
Labels
component/expression sig/execution SIG execution 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
Milestone
v4.0.0-rc.2
Development

Successfully merging this pull request may close these issues.

plan cache: the range partition table's query plan is not optimal
6 participants
@imtbkcat @tiancaiamao @sre-bot @eurekaka @zz-jason @crazycs520