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: make CREATE STATISTICS work properly with EXPLAIN ANALYZE (DEBUG) #57606

Merged
merged 4 commits into from
Dec 8, 2020

Conversation

RaduBerinde
Copy link
Member

sql: migrate other EXPLAIN variants to new exec factory interface

This change migrates the other EXPLAIN variants to the new interface,
which uses an explain factory to build the explained plan.

This fixes a TODO in the experimental factory.

Release note: None

opt: add operator for CREATE STATISTICS

This change adds an optimizer operator for CREATE STATISTICS (instead
of using the opaque operator).

Release note: None

sql: reimplement EXPLAIN handling of CREATE STATISTICS

CREATE STATISTICS runs the statistics plan inside a job. The plan node
tree ends in a dead end at the createStatsNode. For explain purposes,
we want to see information about the actual plan that runs inside the
job.

The current solution is to pretend that we build the plan directly in
certain contexts. This solution is hard to extend for the new
top-level instrumentation for EXPLAIN ANALYZE.

This commit changes this solution to add a runAsJob flag and
actually plan the distributed plan directly when we don't run as a
job. We automatically set runAsJob=false whenever we are building
inside Explain.

Release note: None

sql: make CREATE STATISTICS work properly with EXPLAIN ANALYZE (DEBUG)

With this change, the bundle now contains the diagram for the
underlying plan.

Release note: None

@RaduBerinde RaduBerinde requested a review from rytaft December 5, 2020 14:14
@RaduBerinde RaduBerinde requested a review from a team as a code owner December 5, 2020 14:14
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@rytaft rytaft 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 6 of 6 files at r1, 16 of 16 files at r2, 7 of 7 files at r3, 2 of 2 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)


pkg/sql/instrumentation.go, line 264 at r4 (raw file):

// EXPLAIN ANALYZE, in which case we want to run any CREATE STATISTICS plan
// directly (and not through a job).
func (ih *instrumentationHelper) ShouldInlineCreateStatsJob() bool {

[nit] this name doesn't really explain what this function does. "inline" seems a bit far from "don't run as a job".

This change migrates the other EXPLAIN variants to the new interface,
which uses an explain factory to build the explained plan.

This fixes a TODO in the experimental factory.

Release note: None
This change adds an optimizer operator for CREATE STATISTICS (instead
of using the opaque operator).

Release note: None
CREATE STATISTICS runs the statistics plan inside a job. The plan node
tree ends in a dead end at the createStatsNode. For explain purposes,
we want to see information about the actual plan that runs inside the
job.

The current solution is to pretend that we build the plan directly in
certain contexts. This solution is hard to extend for the new
top-level instrumentation for EXPLAIN ANALYZE.

This commit changes this solution to add a `runAsJob` flag and
actually plan the distributed plan directly when we don't run as a
job. We automatically set `runAsJob=false` whenever we are building
inside Explain.

Release note: None
With this change, the bundle now contains the diagram for the
underlying plan.

Release note: None
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.

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


pkg/sql/instrumentation.go, line 264 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] this name doesn't really explain what this function does. "inline" seems a bit far from "don't run as a job".

Done. Inverted to ShouldUseCreateStatsJob().

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 16 files at r6, 6 of 7 files at r7, 2 of 2 files at r8.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 8, 2020

Build succeeded:

@craig craig bot merged commit 09aa7c8 into cockroachdb:master Dec 8, 2020
@RaduBerinde RaduBerinde deleted the analyze-create-stats branch December 16, 2020 22:58
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.

3 participants