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

sql: introduce EXPLAIN ANALYZE (PLAN) #56524

Merged
merged 2 commits into from Nov 12, 2020

Conversation

RaduBerinde
Copy link
Member

sql: introduce EXPLAIN ANALYZE (PLAN)

This change introduces a new variant of EXPLAIN ANALYZE which
executes the query and returns the execution plan (in the future,
annotated with stats from execution).

Note that (PLAN) is the default mode for the regular EXPLAIN.
Currently EXPLAIN ANALYZE defaults to (DISTSQL) mode, but we plan
to switch that default to the (PLAN) version once it is fully
implemented.

Release note (sql change): Added a new variant of explain: EXPLAIN
ANALYZE (PLAN).

sql: add planning and execution time to EXPLAIN ANALYZE (PLAN)

This commit adds planning and execution time as top-level fields in
EXPLAIN ANALYZE (PLAN). We also plumb a testing knob to allow these
fields to be deterministic in tests.

Release note: None

image

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde RaduBerinde changed the title sql: add planning and execution time to EXPLAIN ANALYZE (PLAN) sql: introduce EXPLAIN ANALYZE (PLAN) Nov 10, 2020
@awoods187
Copy link
Contributor

So exciting!!

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

Reviewed 9 of 9 files at r1, 5 of 5 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @RaduBerinde)


pkg/sql/conn_executor_exec.go, line 351 at r1 (raw file):

			ih.SetOutputMode(explainAnalyzeDebugOutput, explain.Flags{})
		} else {
			flags := explain.MakeFlags(&e.ExplainOptions)

Should we increment sqltelemetry.ExplainAnalyzeUseCounter here?


pkg/sql/exec_util.go, line 834 at r2 (raw file):

	// EXPLAIN ANALYZE (PLAN) that can vary between runs (like elapsed times).
	// Should be set together with execinfra.TestingKnobs.DeterministicStats.
	DeterministicExplainAnalyze bool

Should we phase out DeterministicStats in favor of this one?


pkg/sql/instrumentation.go, line 302 at r2 (raw file):

// planRows generates the plan tree as a list of strings (one for each line).
// Used in explainAnalyzePlanOutput mode.
func (ih *instrumentationHelper) planRows(phaseTimes *phaseTimes) []string {

nit: if this is only used for explain analyze, what do you think of renaming this method to planRowsForExplainAnalyze similar to planStringForBundle above?

@RaduBerinde RaduBerinde force-pushed the explain-analyze-plan branch 2 times, most recently from 6aca628 to 2376405 Compare November 11, 2020 18:09
Copy link
Member Author

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)


pkg/sql/conn_executor_exec.go, line 351 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Should we increment sqltelemetry.ExplainAnalyzeUseCounter here?

Done.


pkg/sql/exec_util.go, line 834 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

Should we phase out DeterministicStats in favor of this one?

I'd like there to be only one too, but they're knobs for different layers. I added a TODO for now.


pkg/sql/instrumentation.go, line 302 at r2 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: if this is only used for explain analyze, what do you think of renaming this method to planRowsForExplainAnalyze similar to planStringForBundle above?

Good idea, done.

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

@RaduBerinde
Copy link
Member Author

TFTR!

bors r+

This change introduces a new variant of `EXPLAIN ANALYZE` which
executes the query and returns the execution plan (in the future,
annotated with stats from execution).

Note that `(PLAN)` is the default mode for the regular `EXPLAIN`.
Currently `EXPLAIN ANALYZE` defaults to `(DISTSQL)` mode, but we plan
to switch that default to the `(PLAN)` version once it is fully
implemented.

Release note (sql change): Added a new variant of explain: EXPLAIN
ANALYZE (PLAN).
This commit adds planning and execution time as top-level fields in
EXPLAIN ANALYZE (PLAN). We also plumb a testing knob to allow these
fields to be deterministic in tests.

Release note: None
@craig
Copy link
Contributor

craig bot commented Nov 12, 2020

Canceled.

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 12, 2020

Build succeeded:

@craig craig bot merged commit 6de9bf8 into cockroachdb:master Nov 12, 2020
@RaduBerinde RaduBerinde deleted the explain-analyze-plan branch November 16, 2020 21:32
craig bot pushed a commit that referenced this pull request Apr 14, 2021
63351: Updating syntax diagram for EXPLAIN ANALYZE (PLAN). r=RaduBerinde a=ianjevans

Related to #56524.

Updates the syntax diagram and BNF for `EXPLAIN ANALYZE (PLAN)`.

Release note: None

Co-authored-by: ianjevans <ian@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants