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

tidb: support a plan cache for prepared statements #3956

Merged
merged 10 commits into from Oct 24, 2017

Conversation

Projects
None yet
@dbjoa
Contributor

dbjoa commented Jul 31, 2017

Since prepared statements are compiled and optimized whenever they are executed, there is a room to reduce the execution time of the prepared statements.

In order to do that we can compile a prepared statement once and reuse the compiled plan by converting parameter expressions to deferred ones, which are evaluated as late as possible, and storing the plan to a cache.

The implentation has some limitations:

  • the prepared statements with limit expressions are not cached
  • the statistics related to the cache are not supported
  • on-demand methods for invalidating the cache are not supported
@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jul 31, 2017

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Jul 31, 2017

CLA assistant check
All committers have signed the CLA.

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Jul 31, 2017

Member

@dbjoa Thanks for your PR!
This is such a big PR. Could you split it into a couple of smaller ones?

Member

shenli commented Jul 31, 2017

@dbjoa Thanks for your PR!
This is such a big PR. Could you split it into a couple of smaller ones?

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Jul 31, 2017

Contributor

Sorry, I could not split the PR into smaller ones because the components changed are correlated for resolving the one problem except for the cache package.

When reviewing the PR, please consider that the cache package (533 lines) is an implementation of a LRU cache by removing mutex operation from the original codes(https://github.com/youtube/vitess/tree/master/go/cache).

Contributor

dbjoa commented Jul 31, 2017

Sorry, I could not split the PR into smaller ones because the components changed are correlated for resolving the one problem except for the cache package.

When reviewing the PR, please consider that the cache package (533 lines) is an implementation of a LRU cache by removing mutex operation from the original codes(https://github.com/youtube/vitess/tree/master/go/cache).

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Jul 31, 2017

Member

OK, we will review the PR. It will take some time.
Please sign the CLA.

Member

shenli commented Jul 31, 2017

OK, we will review the PR. It will take some time.
Please sign the CLA.

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Aug 1, 2017

Member

@dbjoa Is there any benchmark to prove the performance improvement?

Member

shenli commented Aug 1, 2017

@dbjoa Is there any benchmark to prove the performance improvement?

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Aug 1, 2017

Contributor

Here are the capture of TiDB monitoring dashboard to show the performance gap relatively.
In the figure, "w/o plan cache" denotes the QPS for processing prepared statements without the plan cache, and "w/ plan cache" does the QPS with the plan cache.

The figure shows that the performance gain is about 27%.

Test Environment: instances are built from OpenStack(kilo)

  • TiDB : 3 x c3.4xlarge
  • TiKV : 4 x c3.4xlarge
  • PD : 3 x c3.large
  • HAProxy: 3 x c3.large
  • YCSB : 4 x c3.2xlarge

Test Workload (60K OPS --> TiDB Cluster)

  • target : 15K OPS / YCSB
  • thread : 150 / YCSB
  • readproportion : 1 (read only)
  • workload=com.yahoo.ycsb.workloads.CoreWorkload

Test DB

  • recordcount=30000000
  • fieldcount=10
  • fieldlength=100

Git Hash: 50350f7 (17.6.26)

tidb-plan-cache-perf-tidb-3-tikv-4-tipd-3-17-07-17

Contributor

dbjoa commented Aug 1, 2017

Here are the capture of TiDB monitoring dashboard to show the performance gap relatively.
In the figure, "w/o plan cache" denotes the QPS for processing prepared statements without the plan cache, and "w/ plan cache" does the QPS with the plan cache.

The figure shows that the performance gain is about 27%.

Test Environment: instances are built from OpenStack(kilo)

  • TiDB : 3 x c3.4xlarge
  • TiKV : 4 x c3.4xlarge
  • PD : 3 x c3.large
  • HAProxy: 3 x c3.large
  • YCSB : 4 x c3.2xlarge

Test Workload (60K OPS --> TiDB Cluster)

  • target : 15K OPS / YCSB
  • thread : 150 / YCSB
  • readproportion : 1 (read only)
  • workload=com.yahoo.ycsb.workloads.CoreWorkload

Test DB

  • recordcount=30000000
  • fieldcount=10
  • fieldlength=100

Git Hash: 50350f7 (17.6.26)

tidb-plan-cache-perf-tidb-3-tikv-4-tipd-3-17-07-17

@dbjoa dbjoa closed this Aug 1, 2017

@dbjoa dbjoa reopened this Aug 1, 2017

@blacktear23

This comment has been minimized.

Show comment
Hide comment
@blacktear23

blacktear23 Aug 1, 2017

Contributor

I have a question about Plan Cache. For now TiDB will generate a bad performance plan (Full table scan) when SQL like select * from testtable where intcolumn > "1" limit 10;. The generated plan will cast intcolumn to tpReal. For now I have do some job focusing on how to optimize type cast (#3924). If we change "1" to a placeholder like "?" as a deferred constant how can we guess this constant's type and convert it to fit column type?

Contributor

blacktear23 commented Aug 1, 2017

I have a question about Plan Cache. For now TiDB will generate a bad performance plan (Full table scan) when SQL like select * from testtable where intcolumn > "1" limit 10;. The generated plan will cast intcolumn to tpReal. For now I have do some job focusing on how to optimize type cast (#3924). If we change "1" to a placeholder like "?" as a deferred constant how can we guess this constant's type and convert it to fit column type?

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Aug 1, 2017

Contributor

@blacktear23

For a performance perspective, we shoud cast the parameter to int type. In order to that the default type of the parameter can be string like in a commercial database system. i.e., the expression, intcolumn > ?, can be converted into GT(intcolumn, to_int(getparam("0")). Here "0" means the index of the first parameter and the return type of getparam function is string.

Here is the data type precedence of SAP HANA, which will be helpful for your work:
https://help.sap.com/viewer/4fe29514fd584807ac9f2a04f6754767/2.0.01/en-US/46ff9650c7f44461a6146269c1e2a4c6.html

Contributor

dbjoa commented Aug 1, 2017

@blacktear23

For a performance perspective, we shoud cast the parameter to int type. In order to that the default type of the parameter can be string like in a commercial database system. i.e., the expression, intcolumn > ?, can be converted into GT(intcolumn, to_int(getparam("0")). Here "0" means the index of the first parameter and the return type of getparam function is string.

Here is the data type precedence of SAP HANA, which will be helpful for your work:
https://help.sap.com/viewer/4fe29514fd584807ac9f2a04f6754767/2.0.01/en-US/46ff9650c7f44461a6146269c1e2a4c6.html

@tiancaiamao

Nice proposal! You really did a lot of work in this commit. @dbjoa

Show outdated Hide outdated plan/physical_plan_builder.go
Show outdated Hide outdated util/cache/unsafe_lru_cache.go
@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Aug 27, 2017

Member

@dbjoa Thanks! We are ready to review your PR.

Member

shenli commented Aug 27, 2017

@dbjoa Thanks! We are ready to review your PR.

Show outdated Hide outdated plan/validator.go
Show outdated Hide outdated plan/validator.go
Show outdated Hide outdated executor/prepared.go
Show outdated Hide outdated executor/prepared.go
Show outdated Hide outdated sessionctx/variable/session.go
Show outdated Hide outdated plan/physical_plans.go
Show outdated Hide outdated plan/physical_plans.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated expression/builtin_other.go
Show outdated Hide outdated plan/expression_rewriter.go
Show outdated Hide outdated expression/typeinferer.go
Show outdated Hide outdated executor/prepared.go
Show outdated Hide outdated executor/prepared.go
@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Aug 30, 2017

Contributor

@zz-jason ,
Sorry, there seems to be new bugs in the modified codes. Please pause the review until the bugs are fixed.
I'll let you know when to resume the review.

Contributor

dbjoa commented Aug 30, 2017

@zz-jason ,
Sorry, there seems to be new bugs in the modified codes. Please pause the review until the bugs are fixed.
I'll let you know when to resume the review.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 30, 2017

ok

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Sep 1, 2017

Contributor

@zz-jason,
I'v fixed the issues. The new executors (i.e., TableReaderExecutor, IndexReaderExecutor, IndexLookUpExecutor for new_distsql) should be supported.

Please resume the review.

Contributor

dbjoa commented Sep 1, 2017

@zz-jason,
I'v fixed the issues. The new executors (i.e., TableReaderExecutor, IndexReaderExecutor, IndexLookUpExecutor for new_distsql) should be supported.

Please resume the review.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 1, 2017

Member

ok

Member

zz-jason commented Sep 1, 2017

ok

Show outdated Hide outdated executor/distsql.go
Show outdated Hide outdated expression/builtin.go
Show outdated Hide outdated expression/builtin.go
Show outdated Hide outdated context/context.go
Show outdated Hide outdated expression/expression.go
Show outdated Hide outdated expression/constant_fold.go
Show outdated Hide outdated expression/constant_fold.go
Show outdated Hide outdated expression/expr_to_pb.go
Show outdated Hide outdated plan/optimizer.go
Show outdated Hide outdated plan/optimizer.go
Show outdated Hide outdated executor/prepared.go
Show outdated Hide outdated expression/builtin.go
@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 1, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot commented Sep 1, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 1, 2017

Member

/ok-to-test

Member

iamxy commented Sep 1, 2017

/ok-to-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 5, 2017

Member

/run-all-test

Member

zz-jason commented Sep 5, 2017

/run-all-test

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 5, 2017

Member

/run-all-test

Member

iamxy commented Sep 5, 2017

/run-all-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 8, 2017

Member

@dbjoa any updates ?

Member

zz-jason commented Sep 8, 2017

@dbjoa any updates ?

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 23, 2017

Member

/run-common-test

Member

zz-jason commented Oct 23, 2017

/run-common-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 23, 2017

Member

/run-all-test

Member

zz-jason commented Oct 23, 2017

/run-all-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 23, 2017

Member

/run-integration-ddl-test

Member

zz-jason commented Oct 23, 2017

/run-integration-ddl-test

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Oct 23, 2017

Contributor

@zz-jason
Thank you for your help!

Contributor

dbjoa commented Oct 23, 2017

@zz-jason
Thank you for your help!

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 23, 2017

Member

@dbjoa Congratulations! We'd better disable the plan cache for prepared statements by default.

Member

zz-jason commented Oct 23, 2017

@dbjoa Congratulations! We'd better disable the plan cache for prepared statements by default.

Show outdated Hide outdated context/context.go
Show outdated Hide outdated executor/executor_test.go
Show outdated Hide outdated expression/constant.go
Show outdated Hide outdated expression/expr_to_pb.go
@@ -1044,6 +1051,9 @@ func createSession(store kv.Storage) (*session, error) {
parser: parser.New(),
sessionVars: variable.NewSessionVars(),
}
if cache.PreparedPlanCacheEnabled {

This comment has been minimized.

@zz-jason

zz-jason Oct 23, 2017

Member

we should initialize s.preparedPlanCache when creating a session, not here .

@zz-jason

zz-jason Oct 23, 2017

Member

we should initialize s.preparedPlanCache when creating a session, not here .

This comment has been minimized.

@dbjoa

dbjoa Oct 23, 2017

Contributor

Is CreateSession() right place?

@dbjoa

dbjoa Oct 23, 2017

Contributor

Is CreateSession() right place?

This comment has been minimized.

@zz-jason

zz-jason Oct 23, 2017

Member

yes, in createSession and createSessionWithDomain

@zz-jason

zz-jason Oct 23, 2017

Member

yes, in createSession and createSessionWithDomain

This comment has been minimized.

@dbjoa

dbjoa Oct 23, 2017

Contributor

Thank you for your comments.

@dbjoa

dbjoa Oct 23, 2017

Contributor

Thank you for your comments.

cache.GlobalPlanCache = kvcache.NewShardedLRUCache(cache.PlanCacheCapacity, cache.PlanCacheShards)
}
cache.PreparedPlanCacheEnabled = cfg.PreparedPlanCache.Enabled

This comment has been minimized.

@zz-jason

zz-jason Oct 23, 2017

Member

cache.PreparedPlanCacheEnabled should be initialized when tidb-server is started, not here.

@zz-jason

zz-jason Oct 23, 2017

Member

cache.PreparedPlanCacheEnabled should be initialized when tidb-server is started, not here.

This comment has been minimized.

@dbjoa

dbjoa Oct 23, 2017

Contributor

Since Prepared Plan Cache will be initialized at CreateSession(), I think that setGlobalVars() can a right place. Would you suggest a better place?

@dbjoa

dbjoa Oct 23, 2017

Contributor

Since Prepared Plan Cache will be initialized at CreateSession(), I think that setGlobalVars() can a right place. Would you suggest a better place?

This comment has been minimized.

@zz-jason

zz-jason Oct 23, 2017

Member

never mind, sorry for this wrong comment

@zz-jason

zz-jason Oct 23, 2017

Member

never mind, sorry for this wrong comment

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 24, 2017

Member

/run-all-test

Member

zz-jason commented Oct 24, 2017

/run-all-test

Show outdated Hide outdated config/config.go

dbjoa added some commits Oct 13, 2017

tidb: support a plan cache for prepared statements
      - compile a prepared statement once and reuse the compiled plan
      - only evaluate expression including parameterized ones whenever execute the prepared statement
      - use a LRU cache lib. for storing the plan (#4644)
tidb: fix the two issues when prepared statements with null parameters
  - incorrect plans generated when the null parameters are given at the first execution time
  - generated plans are not optimal due to the wrong placed type casts
expression,util: remove ETParam
  - two tests failed
  - but it will be fixed by making TypeUnspecified to act like ETParam
expression,util: fix the two fails
  - should make TypeUnspecified to be lower the priority than other types
tidb: refactoring
  - change the prepared-plan-cache key generation logic
  - consolidate the cacheable check logics
  - move prepared-plan related prameters to config/config.go and config.toml.example
  - change a function name
  - fix a crash bug and add tests
executor, expression: fix CI errors
 - ranges should use the profer Column Infos of each table or index
 - constant with a valid DeferredExpr should be set its default type from its values
config: disable the plan cache for prepare statements by default
  - enable the plan cache when running prepared query tests
@zz-jason

LGTM

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Oct 24, 2017

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Oct 24, 2017

Member

Well done. Thank you for your great work @dbjoa

Member

ngaut commented Oct 24, 2017

Well done. Thank you for your great work @dbjoa

@ngaut ngaut merged commit 0306bb0 into pingcap:master Oct 24, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Oct 24, 2017

Member

@dbjoa Thanks for your amazing work in the past three months!

Member

shenli commented Oct 24, 2017

@dbjoa Thanks for your amazing work in the past three months!

@dbjoa

This comment has been minimized.

Show comment
Hide comment
@dbjoa

dbjoa Oct 24, 2017

Contributor

@zz-jason @coocood
Thank you very much for your insightful and detailed review comments.

Contributor

dbjoa commented Oct 24, 2017

@zz-jason @coocood
Thank you very much for your insightful and detailed review comments.

dbjoa added a commit to cloud-pi/tidb that referenced this pull request Oct 24, 2017

tidb: support a plan cache for prepared statements (#3956)
* tidb: support a plan cache for prepared statements
@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Oct 24, 2017

Member

@dbjoa
Thank you for your great contribution!

Member

coocood commented Oct 24, 2017

@dbjoa
Thank you for your great contribution!

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Oct 24, 2017

Member

@dbjoa Thank you for your impressive contribution, you really did a great work!

Member

zz-jason commented Oct 24, 2017

@dbjoa Thank you for your impressive contribution, you really did a great work!

@dbjoa dbjoa referenced this pull request Oct 25, 2017

Merged

excutor: tiny code cleanup #4899

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