sql: use query cache for prepare #33575

RaduBerinde · 2019-01-08T22:35:33Z

Note: the "refactor usage of placeholder TypeHints" commit is from a separate PR, ignore that one.

sql: separate prepared statement metadata

Separating the metadata for prepared statements in a sqlbase struct,
which will also be used by the plan cache.

Release note: None

sql: use query cache for prepare

This change enables the use of the query cache for Prepare. The cache
optionally stores a *PrepareMetadata for each query which can later
be reused.

Release note: None

cockroach-teamcity · 2019-01-08T22:35:42Z

This change is

rytaft

very nice!

Reviewed 9 of 9 files at r1, 3 of 3 files at r2, 8 of 8 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/plan_opt_test.go, line 146 at r3 (raw file):

				r.CheckQueryResults(
					t,
					fmt.Sprintf("EXECUTE a%d (10, 100)", i),

Should this still work with other params? If so, maybe add a couple of different params...

pkg/sql/sqlbase/prepared_statement.go, line 1 at r2 (raw file):

// Copyright 2016 The Cockroach Authors.

2016 -> 2019!

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained

pkg/sql/plan_opt_test.go, line 146 at r3 (raw file):

Previously, rytaft wrote…

Should this still work with other params? If so, maybe add a couple of different params...

You mean with other values for the arguments (the 10, 100)?

rytaft · 2019-01-09T21:26:35Z

pkg/sql/plan_opt_test.go, line 146 at r3 (raw file):

Previously, RaduBerinde wrote…

You mean with other values for the arguments (the 10, 100)?

Yea -- sorry, that wasn't clear

andy-kimball

, nice work.

Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/conn_executor_prepare.go, line 225 at r3 (raw file):

	}

	// PREPARE has a limited subset of statements it can be run with. Postgres

I know this is not related to this PR, but we now support CREATE TABLE. How will a DDL statement like that interact with the PREPARE logic?

pkg/sql/plan_opt.go, line 43 at r3 (raw file):

func (p *planner) prepareUsingOptimizer(
	ctx context.Context, stmt Statement,
) (_ planFlags, isCorrelated bool, _ error) {

Should isCorrelated be part of the plan flags?

pkg/sql/plan_opt.go, line 54 at r3 (raw file):

		cachedData, ok := p.execCfg.QueryCache.Find(stmt.SQL)
		if ok && cachedData.PrepareMetadata != nil {
			pm := cachedData.PrepareMetadata

Should be &cachedData.PrepareMetadata, because it's a huge struct.

pkg/sql/prepared_stmt.go, line 37 at r3 (raw file):

	// information.
	Str string
	// AnonymizedStr is the anonymized statement string suitable for recording

Nice to see this particular mess cleaned up.

pkg/sql/sqlbase/prepared_statement.go, line 35 at r3 (raw file):

	Statement tree.Statement

	tree.PlaceholderTypesInfo

NIT: Would be nice to have comments on these other fields.

RaduBerinde

Reviewable status: complete! 2 of 0 LGTMs obtained

pkg/sql/conn_executor_prepare.go, line 225 at r3 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I know this is not related to this PR, but we now support CREATE TABLE. How will a DDL statement like that interact with the PREPARE logic?

Good question. I can't think of a problem OTOH, but regardless I think we should limit the preparable statements to what postgres supports. I'll follow up on this.

pkg/sql/plan_opt.go, line 43 at r3 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Should isCorrelated be part of the plan flags?

It's a little different in that it's set in an error path (in fact it only matters when there's an error). I don't like it either, I wish there was a way to associate this info with the error.

pkg/sql/plan_opt.go, line 54 at r3 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Should be &cachedData.PrepareMetadata, because it's a huge struct.

It's already a pointer.

pkg/sql/plan_opt_test.go, line 146 at r3 (raw file):

Previously, rytaft wrote…

Yea -- sorry, that wasn't clear

Added.

pkg/sql/sqlbase/prepared_statement.go, line 1 at r2 (raw file):

Previously, rytaft wrote…

2016 -> 2019!

Done.

rytaft

Reviewed 8 of 8 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

andy-kimball

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale)

pkg/sql/plan_opt.go, line 54 at r3 (raw file):

Previously, RaduBerinde wrote…

It's already a pointer.

Ah, I was thinking this was from the PreparedStatement.

Separating the metadata for prepared statements in a sqlbase struct, which will also be used by the plan cache. Release note: None

This change enables the use of the query cache for Prepare. The cache optionally stores a `*PrepareMetadata` for each query which can later be reused. Release note: None

RaduBerinde · 2019-01-10T19:47:20Z

bors r+

33575: sql: use query cache for prepare r=RaduBerinde a=RaduBerinde Note: the "refactor usage of placeholder TypeHints" commit is from a separate PR, ignore that one. #### sql: separate prepared statement metadata Separating the metadata for prepared statements in a sqlbase struct, which will also be used by the plan cache. Release note: None #### sql: use query cache for prepare This change enables the use of the query cache for Prepare. The cache optionally stores a `*PrepareMetadata` for each query which can later be reused. Release note: None Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

craig · 2019-01-10T20:11:09Z

Build succeeded

GitHub CI (Cockroach)

Fixes #4334. Summary of changes: - Update 'Query plan cache' section of the CBO page as follows: - Query cache is now enabled by default - BUGFIX: Updated example to use `SET CLUSTER SETTING`, was using `SET` - Removed the following limitations since (if I'm reading PRs correctly) they no longer exist: - Invalidate cached plans when stats change (cockroachdb/cockroach#34057) - Query cache works for prepared stmts (cockroachdb/cockroach#33575)

RaduBerinde requested review from andy-kimball and a team January 8, 2019 22:35

RaduBerinde requested a review from a team as a code owner January 8, 2019 22:35

RaduBerinde requested review from a team January 8, 2019 22:35

RaduBerinde force-pushed the query-cache-prepare branch from efed39a to fd834a3 Compare January 8, 2019 22:58

rytaft approved these changes Jan 9, 2019

View reviewed changes

RaduBerinde commented Jan 9, 2019

View reviewed changes

andy-kimball approved these changes Jan 9, 2019

View reviewed changes

RaduBerinde force-pushed the query-cache-prepare branch from fd834a3 to 7b0bb39 Compare January 10, 2019 00:57

RaduBerinde commented Jan 10, 2019

View reviewed changes

rytaft approved these changes Jan 10, 2019

View reviewed changes

andy-kimball approved these changes Jan 10, 2019

View reviewed changes

RaduBerinde added 2 commits January 10, 2019 14:15

sql: separate prepared statement metadata

c14a126

Separating the metadata for prepared statements in a sqlbase struct, which will also be used by the plan cache. Release note: None

sql: use query cache for prepare

e007bc6

This change enables the use of the query cache for Prepare. The cache optionally stores a `*PrepareMetadata` for each query which can later be reused. Release note: None

RaduBerinde force-pushed the query-cache-prepare branch from 7b0bb39 to e007bc6 Compare January 10, 2019 19:15

craig bot merged commit e007bc6 into cockroachdb:master Jan 10, 2019

RaduBerinde deleted the query-cache-prepare branch January 10, 2019 20:19

rmloveland mentioned this pull request Feb 7, 2019

Document that optimizer query cache is on by default for v2.2.0-alpha-20190211 cockroachdb/docs#4334

Closed

rmloveland mentioned this pull request Feb 7, 2019

Document that query cache is enabled by default cockroachdb/docs#4335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: use query cache for prepare #33575

sql: use query cache for prepare #33575

RaduBerinde commented Jan 8, 2019

cockroach-teamcity commented Jan 8, 2019

rytaft left a comment

RaduBerinde left a comment

rytaft commented Jan 9, 2019

andy-kimball left a comment

RaduBerinde left a comment

rytaft left a comment

andy-kimball left a comment

RaduBerinde commented Jan 10, 2019

craig bot commented Jan 10, 2019

sql: use query cache for prepare #33575

sql: use query cache for prepare #33575

Conversation

RaduBerinde commented Jan 8, 2019

sql: separate prepared statement metadata

sql: use query cache for prepare

cockroach-teamcity commented Jan 8, 2019

rytaft left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft commented Jan 9, 2019

andy-kimball left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

RaduBerinde commented Jan 10, 2019

craig bot commented Jan 10, 2019

Build succeeded