executor: new execution path for point execution within prepared plan cache by cfzjywxk · Pull Request #11970 · pingcap/tidb

cfzjywxk · 2019-09-02T02:47:58Z

What problem does this PR solve?

move plan cache try fetching at start for prepare statement, used for optimizations based on this, like point select or maybe other code paths.(this will be done for non prepared statement with a more universal plan cache in the future)
for prepare plan cache execution path, make a short path for point select skipping unnecessary things like ast traverse, tso fetch etc.
simplify optimization process of execution statement when plan cache hit
make point get plan cache logic seperated with Prepare statement plan cache utils, point get plan will be cached and executed not affected by the prepared-plan-cache switch, the parser related pull request executor: cache point get plan for prepared point parser#541

sysbench with 1million rows 16 tables oltp_point_select, single tidb server instance
prepare plan cache enabled

branch	qps
dev branch	queries: 154567.64 per sec
master branch	queries: 132717.35 per sec

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Release note

codecov · 2019-09-02T02:52:31Z

Codecov Report

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

@@             Coverage Diff             @@
##             master     #11970   +/-   ##
===========================================
  Coverage   81.3664%   81.3664%           
===========================================
  Files           454        454           
  Lines         99777      99777           
===========================================
  Hits          81185      81185           
  Misses        12840      12840           
  Partials       5752       5752

ngaut · 2019-09-02T03:35:39Z

PTAL @dbjoa

ngaut · 2019-09-02T03:51:04Z

/bench

sre-bot · 2019-09-02T05:35:32Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 2aa3e27a6d535046efe789513d2fb60b474c5900
+++ tidb: 7f9aa0efe4fb91718006d4a53640bb9f4ca202e4
tikv: af70f367a38aad3305e32d8de17f60e09ea87fc3
pd: a1f8d9db4ac3e713c3259ca0979312fe3b61adac
================================================================================
test-1: < oltp_insert >
    * QPS : 21549.68 ± 0.1964% (std=31.40) delta: -0.48%
    * AvgMs : 11.87 ± 0.1965% (std=0.02) delta: 0.47%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.00%
            
test-2: < oltp_update_non_index >
    * QPS : 29577.49 ± 0.3464% (std=68.69) delta: 0.05%
    * AvgMs : 8.65 ± 0.3699% (std=0.02) delta: -0.07%
    * PercentileMs99 : 30.59 ± 1.0788% (std=0.27) delta: 0.36%
            
test-3: < oltp_read_write >
    * QPS : 33062.96 ± 0.1520% (std=30.74) delta: 0.09%
    * AvgMs : 155.55 ± 0.1414% (std=0.13) delta: -0.10%
    * PercentileMs99 : 287.38 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 75946.31 ± 0.6928% (std=333.75) delta: 0.47%
    * AvgMs : 3.37 ± 0.5935% (std=0.01) delta: -0.53%
    * PercentileMs99 : 7.40 ± 2.1070% (std=0.10) delta: 0.35%
            
test-5: < oltp_update_index >
    * QPS : 17111.05 ± 0.3533% (std=35.73) delta: -0.01%
    * AvgMs : 14.96 ± 0.3209% (std=0.03) delta: 0.00%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: 0.73%

https://perf.pingcap.com

jackysp · 2019-09-02T06:35:59Z

@zhouqiang-cl, is the plan cache enabled on our bench?

mahjonp · 2019-09-02T06:56:12Z

@zhouqiang-cl, is the plan cache enabled on our bench?

Not enable yet, use the default value now. If we enable plan cache, what's the effect of sysbench benchmark?

mahjonp · 2019-09-02T08:57:34Z

/bench again after enable plan cache

sre-bot · 2019-09-02T12:05:50Z

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: b239f2f04d3aa19a732356b16e12796806f5f2d4
+++ tidb: 7f9aa0efe4fb91718006d4a53640bb9f4ca202e4
tikv: af70f367a38aad3305e32d8de17f60e09ea87fc3
pd: a1f8d9db4ac3e713c3259ca0979312fe3b61adac
================================================================================
test-1: < oltp_insert >
    * QPS : 21633.61 ± 0.1336% (std=25.91) delta: 0.79%
    * AvgMs : 11.83 ± 0.1268% (std=0.01) delta: -0.80%
    * PercentileMs99 : 42.61 ± 0.0000% (std=0.00) delta: 0.00%
            
test-2: < oltp_update_non_index >
    * QPS : 30287.80 ± 0.4567% (std=85.50) delta: 0.19%
    * AvgMs : 8.45 ± 0.4498% (std=0.02) delta: -0.24%
    * PercentileMs99 : 30.26 ± 0.0000% (std=0.00) delta: -0.72%
            
test-3: < oltp_read_write >
    * QPS : 34709.85 ± 0.2059% (std=47.14) delta: 0.42%
    * AvgMs : 148.18 ± 0.2038% (std=0.20) delta: -0.42%
    * PercentileMs99 : 292.60 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 83757.10 ± 1.6038% (std=1052.79) delta: 10.30%
    * AvgMs : 3.06 ± 1.5052% (std=0.04) delta: -9.05%
    * PercentileMs99 : 5.88 ± 1.8707% (std=0.10) delta: -19.45%
            
test-5: < oltp_update_index >
    * QPS : 17532.85 ± 0.1740% (std=27.29) delta: 0.10%
    * AvgMs : 14.58 ± 0.4801% (std=0.04) delta: -0.23%
    * PercentileMs99 : 46.63 ± 0.0000% (std=0.00) delta: -0.36%

https://perf.pingcap.com

zz-jason · 2019-09-02T12:14:00Z

from the benchmark result, it can improve 10% of the QPS on the oltp_point_select scenario?

jackysp · 2019-09-02T12:47:39Z

Yes, it is true, @zz-jason .

lysu · 2019-09-02T12:48:26Z

planner/core/common_plans.go

-	var cacheKey kvcache.Key
-	sessionVars := sctx.GetSessionVars()
-	sessionVars.StmtCtx.UseCache = prepared.UseCache
+	cacheKey := NewPSTMTPlanCacheKey(sctx.GetSessionVars(), e.ExecID, prepared.SchemaVersion)


we can move this line to 255?

lysu · 2019-09-02T13:07:44Z

can tryUpdatePointPlan and tryDeletePointPlan take benefit from this~? @cfzjywxk

lysu · 2019-09-02T13:18:40Z

maybe we should cache point-get plan for prepare even if plan cache is disabled @cfzjywxk ?

executor/adapter.go

cfzjywxk · 2019-09-03T02:31:21Z

maybe we should cache point-get plan for prepare even if plan cache is disabled @cfzjywxk ?

I'm considering how should we do this.

Make a simplified universal and instance level plan cache and support point query. This will speed up such queries no affedted by prepare or non prepare statements.
Still using prepared plan cache, but always cache specific plan(like point plan) despite the switch on/off. This is easier to implement on current code.

@jackysp @lysu any suggestions, thx

jackysp · 2019-09-03T03:46:08Z

2. Still using prepared plan cache, but always cache specific plan(like point plan) despite the switch on/off. This is easier to implement on current code.

I think it is OK for me.

coocood · 2019-09-03T09:12:47Z

@cfzjywxk
The improvement caused by avoiding fetch TSO is also included in this PR #11981

cfzjywxk · 2019-09-03T09:57:10Z

@cfzjywxk
The improvement caused by avoiding fetch TSO is also included in this PR #11981

roger, I think it's better to classify if this execution need "get tso" or sth else like this, so that we do not need do delay tso fetching ?
@coocood @jackysp

… prepared cache enabled or not

cfzjywxk · 2019-09-12T10:02:02Z

@jackysp @lysu PTAL need approvements

executor/adapter.go

zz-jason · 2019-09-14T06:25:52Z

executor/adapter.go

+	}
+	sctx.GetSessionVars().StmtCtx.Priority = kv.PriorityHigh
+	b := newExecutorBuilder(sctx, is)
+	exec := b.build(a.Plan)


How about calling a.buildExecutor() to build the operator tree from the execution plan?

here just keep necessary steps to make it clear for the short path

planner/core/common_plans.go

zz-jason · 2019-09-14T06:47:10Z

planner/optimize.go

+// OptimizeExecStmt to optimize prepare statement protocol "execute" statement
+// this is a short path ONLY does things filling prepare related params
+// for point select like plan which does not need extra things
+func OptimizeExecStmt(ctx context.Context, sctx sessionctx.Context,


I think we should check the privilege of the current user, just like what Optimize() function did. Can we call it directly?

issue https://github.com/pingcap/qa/issues/66 has some explanations on this

cfzjywxk · 2019-09-15T08:00:17Z

/build

coocood

LGTM

sre-bot · 2019-09-17T03:27:17Z

/run-all-tests

sre-bot · 2019-09-17T03:29:21Z

@cfzjywxk merge failed.

zz-jason · 2019-09-17T03:29:26Z

planner/core/common_plans.go

+	if prepared.CachedPlan != nil {
+		// expression rewrite will transfer paramMarker in select.where into Constant Expression
+		// but point get execution dose not need to evaluate where condition,
+		// so prepared.UseCache here false is ok, only for point plan


@yikeke Could you help us to polish this comment?

Ok, working on it.

Done. @zz-jason My version is the translation of the Chinese explanation given by @cfzjywxk ~

planner/core/common_plans.go

yikeke · 2019-09-17T04:51:40Z

planner/core/common_plans.go

 func (e *Execute) getPhysicalPlan(ctx context.Context, sctx sessionctx.Context, is infoschema.InfoSchema, prepared *ast.Prepared) error {
+	if prepared.CachedPlan != nil {
+		// expression rewrite will transfer paramMarker in select.where into Constant Expression
+		// but point get execution dose not need to evaluate where condition,


Suggested change

// but point get execution dose not need to evaluate where condition,

// When Point Select queries are executed,

// the expression in the where condition will not be evaluated,

yikeke · 2019-09-17T04:56:06Z

planner/core/common_plans.go

+	if prepared.CachedPlan != nil {
+		// expression rewrite will transfer paramMarker in select.where into Constant Expression
+		// but point get execution dose not need to evaluate where condition,
+		// so prepared.UseCache here false is ok, only for point plan


Suggested change

// so prepared.UseCache here false is ok, only for point plan

// so you don't need to consider whether prepared.useCache is enabled.

cfzjywxk · 2019-09-17T06:09:28Z

/merge

sre-bot · 2019-09-17T06:09:44Z

/run-all-tests

new execution path for point execution within prepared plan cache

7f9aa0e

cfzjywxk added status/WIP type/performance labels Sep 2, 2019

cfzjywxk requested review from coocood, jackysp, lysu and winoros September 2, 2019 02:47

lysu reviewed Sep 2, 2019

View reviewed changes

Merge branch 'master' into ps2

f6f8a12

lonng reviewed Sep 3, 2019

View reviewed changes

executor/adapter.go Show resolved Hide resolved

cfzjywxk requested a review from imtbkcat September 3, 2019 02:43

Merge branch 'master' into ps2

9b830d2

cfzjywxk added 2 commits September 3, 2019 21:21

store prepared plan for point get, so point using cached plan despite…

020fa89

… prepared cache enabled or not

Merge branch 'master' into ps2

393aebd

Merge branch 'master' into ps2

62052bc

zz-jason reviewed Sep 14, 2019

View reviewed changes

cfzjywxk added 4 commits September 15, 2019 15:26

Merge branch 'master' into ps2

c35ecbf

address comment

b209d43

address comment

1cdfa75

address comment

57033b4

cfzjywxk requested a review from zz-jason September 15, 2019 07:46

Merge branch 'master' into ps2

baef4fa

cfzjywxk mentioned this pull request Sep 16, 2019

*: check privilege when reusing the cached plan #12211

Merged

cfzjywxk removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 16, 2019

Merge branch 'master' into ps2

c422942

cfzjywxk added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 16, 2019

coocood previously approved these changes Sep 17, 2019

View reviewed changes

coocood added the status/can-merge Indicates a PR has been approved by a committer. label Sep 17, 2019

Merge branch 'master' into ps2

dc33f18

zz-jason reviewed Sep 17, 2019

View reviewed changes

yikeke reviewed Sep 17, 2019

View reviewed changes

change comment

0473c5b

cfzjywxk dismissed coocood’s stale review via 0473c5b September 17, 2019 05:13

cfzjywxk added 2 commits September 17, 2019 13:13

Merge branch 'master' into ps2

2c80253

Merge branch 'master' into ps2

75d1ec0

coocood approved these changes Sep 17, 2019

View reviewed changes

sre-bot merged commit 15011b6 into pingcap:master Sep 17, 2019

Reminiscent mentioned this pull request Mar 23, 2022

planner: refactor the code for plan cache #33318

Open

12 tasks

	// but point get execution dose not need to evaluate where condition,
	// When Point Select queries are executed,
	// the expression in the where condition will not be evaluated,

	// so prepared.UseCache here false is ok, only for point plan
	// so you don't need to consider whether prepared.useCache is enabled.

Conversation

cfzjywxk commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Uh oh!

codecov bot commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ngaut commented Sep 2, 2019

Uh oh!

ngaut commented Sep 2, 2019

Uh oh!

sre-bot commented Sep 2, 2019

Uh oh!

jackysp commented Sep 2, 2019

Uh oh!

mahjonp commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahjonp commented Sep 2, 2019

Uh oh!

sre-bot commented Sep 2, 2019

Uh oh!

zz-jason commented Sep 2, 2019

Uh oh!

jackysp commented Sep 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lysu commented Sep 2, 2019

Uh oh!

lysu commented Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cfzjywxk commented Sep 3, 2019

Uh oh!

jackysp commented Sep 3, 2019

Uh oh!

coocood commented Sep 3, 2019

Uh oh!

cfzjywxk commented Sep 3, 2019

Uh oh!

cfzjywxk commented Sep 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfzjywxk commented Sep 15, 2019

Uh oh!

coocood left a comment

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Sep 17, 2019

Uh oh!

sre-bot commented Sep 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfzjywxk commented Sep 2, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading

mahjonp commented Sep 2, 2019 •

edited

Loading

lysu commented Sep 2, 2019 •

edited

Loading