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: add EXPLAIN (VEC) output to the stmt bundle #61887

Merged
merged 2 commits into from
Mar 13, 2021

Conversation

yuzefovich
Copy link
Member

sql: delay saving of flows till after the setup

This commit reorders the code that deals with the flow specifications
a bit. Previously, we would call saveFlows right after generating flow
specifications and we would release the resources back to their pools
once the setup request for each flow has been sent out. However, the
follow-up commit will include the generation of EXPLAIN (VEC, VERBOSE)
diagram into the statement bundle, and to get there we need to delay
both of those actions until we know that the flows are vectorized. As
a result, now saveFlows is called after setupFlows (when we know
whether they were vectorized or not) and the release of the flow specs
is deferred.

Release note: None

sql: add EXPLAIN (VEC) output to the stmt bundle

This commit refactors the code of EXPLAIN (VEC) to be reused to add
(VEC) and (VEC, VERBOSE) flavours to the statement bundle. The code
to print the diagram is moved into colflow package, but other changes
are minor.

The addition of these diagrams is performed on the best effort basis
(meaning that if an error is encountered during populating the diagram,
the error is ignored) because in some edge cases the errors are expected
to occur.

Fixes: #47940.

Release note (sql change): Statement diagnostics bundles now contain
output of EXPLAIN (VEC) and EXPLAIN (VEC, VERBOSE) commands for the
statements.

@yuzefovich yuzefovich requested review from RaduBerinde and a team March 12, 2021 02:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member Author

Confirmed manually that it works:
Screen Shot 2021-03-11 at 6 06 08 PM

Copy link
Member

@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.

Very cool! :lgtm:

The lists of files in TestExplainAnalyzeDebug need updating.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/sql/distsql_physical_planner.go, line 700 at r2 (raw file):

		var explainVecVerbose []string
		if planner.instrumentation.collectBundle && planner.curPlan.flags.IsSet(planFlagVectorized) {
			flowCtx := newFlowCtxForExplainPurposes(p, planner, &planner.extendedEvalCtx.DistSQLPlanner.rpcCtx.ClusterID)

Given that a panic in this code could be very bad, consider adding a panic catcher in this function (you can copy the block in emitExplain()).


pkg/sql/explain_bundle.go, line 324 at r2 (raw file):

			}
			if len(d.explainVec) > 0 {
				b.z.AddFile(fmt.Sprintf(filenameFormat, ""), strings.Join(d.explainVec, "\n"))

[nit] The filenameFormat logic is hard to follow (especially since we're intermixing it with other Sprintfs). I'd do something simpler:

extra := ""
if len() > 1 {
  extra = fmt.Sprintf("-%d-%s")
}
AddFile(fmt.Sprintf("vec%s.txt", extra))
AddFile(fmt.Sprintf("vec%s-v.txt", extra))

@blathers-crl
Copy link

blathers-crl bot commented Mar 12, 2021

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Mar 12, 2021
This commit reorders the code that deals with the flow specifications
a bit. Previously, we would call `saveFlows` right after generating flow
specifications and we would release the resources back to their pools
once the setup request for each flow has been sent out. However, the
follow-up commit will include the generation of `EXPLAIN (VEC, VERBOSE)`
diagram into the statement bundle, and to get there we need to delay
both of those actions until we know that the flows are vectorized. As
a result, now `saveFlows` is called after `setupFlows` (when we know
whether they were vectorized or not) and the release of the flow specs
is deferred.

Release note: None
@yuzefovich yuzefovich removed the O-community Originated from the community label Mar 12, 2021
This commit refactors the code of `EXPLAIN (VEC)` to be reused to add
`(VEC)` and `(VEC, VERBOSE)` flavours to the statement bundle. The code
to print the diagram is moved into `colflow` package, but other changes
are minor.

The addition of these diagrams is performed on the best effort basis
(meaning that if an error is encountered during populating the diagram,
the error is ignored) because in some edge cases the errors are expected
to occur.

Release note (sql change): Statement diagnostics bundles now contain
output of `EXPLAIN (VEC)` and `EXPLAIN (VEC, VERBOSE)` commands for the
statements.
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde)


pkg/sql/distsql_physical_planner.go, line 700 at r2 (raw file):

Previously, RaduBerinde wrote…

Given that a panic in this code could be very bad, consider adding a panic catcher in this function (you can copy the block in emitExplain()).

We already had the vectorized panic catcher in ExplainVec, and I extended it to the whole function body instead of introducing a panic catcher here.

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 13, 2021

Build succeeded:

@craig craig bot merged commit 4d44ddf into cockroachdb:master Mar 13, 2021
@yuzefovich yuzefovich deleted the vec-bundle branch March 15, 2021 17:15
@yuzefovich
Copy link
Member Author

Thoughts on backporting this to 21.1? I think it should be pretty safe and somewhat beneficial.

@jordanlewis
Copy link
Member

I think this is a good idea to backport.

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.

sql: put the output of EXPLAIN (VEC) in the EXPLAIN ANALYZE (DEBUG) bundle
4 participants