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 BUNDLE #45735

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

RaduBerinde
Copy link
Member

This is an initial version of EXPLAIN BUNDLE. Similar to EXPLAIN ANALYZE, it
runs the statement but throws away all resulting rows and instead returns a URL
to a zip bundle (which is served via a special HTTP endpoint); example:

root@127.249.95.44:37247/movr> EXPLAIN BUNDLE SELECT * FROM promo_codes;
                                  text
------------------------------------------------------------------------
  Download the bundle from:
    http://127.249.95.44:35445/_admin/v1/stmtbundle/535092672054657025
  or from the Admin UI (Advanced Debug -> Statement Diagnostics).
(1 row)

The bundle currently contains the statement and EXPLAIN, EXPLAIN (OPT)
plans. The plan is to add (in future changes) the OPT, ENV data, table stats,
EXPLAIN ANALYZE diagram, and traces.

This is not yet integrated with Andrei's query diagnostics work other than using
the same system.table. This will be addressed in future changes; the user will
be able to request a bundle+trace from the UI. Conversely, an EXPLAIN BUNDLE
will also fill out the trace field in the system table.

Note that the AdminUI will have an interface to list and retrieve collected
bundles.

Release note (sql change): EXPLAIN BUNDLE can be used to run a query and collect
execution information in a support bundle, which can be downloaded via the
AdminUI.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde
Copy link
Member Author

@dhartunian @nathanstilwell can you take a look at the http endpoint I set up in admin,go to double check I'm not doing something stupid security-wise?

@RaduBerinde RaduBerinde force-pushed the bundle-prototype branch 2 times, most recently from fb1605d to 0e3a718 Compare March 5, 2020 18:13
Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dhartunian, @jordanlewis, @nathanstilwell, and @RaduBerinde)


pkg/server/admin.go, line 1528 at r1 (raw file):

}

func (s *adminServer) writeStatementBundle(ctx context.Context, id int64, w http.ResponseWriter) {

the "write" in the name seems confusing to me. I understand that we're technically writing something to a writer, but I'd name it more like getStatementBundle.


pkg/server/admin.go, line 1575 at r1 (raw file):

		fmt.Sprintf("attachment; filename=stmt-bundle-%d.zip", id),
	)
	//w.Header().Set("Content-Type", req.Header.Get("Content-Type"))

sup with this?


pkg/server/admin.go, line 1578 at r1 (raw file):

	if _, err := io.Copy(w, &bundle); err != nil {
		// Too late to return an error now..

well, if this fails, it fails because there's no longer anybody to return the error to, right?


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

	case *tree.ExplainBundle:
		stmt.Statement.AST = s.Statement
		// XXX .SQL?

xxx


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

		stmt.Statement.AST = s.Statement
		// XXX .SQL?
		discardRows = true

put a comment here emphasizing that we don't return the rows for the inner query, but instead return a URL. And put a TODO to capture some rows and include them; I think being able to use this functionality through the admin ui to figure out what some requests are returning would be very cool.


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

	}
	if explainBundle {
		if err := p.generateExplainBundle(ctx, res); err != nil {

make a note pls that we're not getting here in case of query execution errors


pkg/sql/explain_bundle.go, line 30 at r1 (raw file):

// generateExplainBundle creates the bundle and returns the result for an
// EXPLAIN BUNDLE statement. The result is text containing a URL for the bundle.
func (p *planner) generateExplainBundle(ctx context.Context, res RestrictedCommandResult) error {

I looked briefly to see who integral the planner is to the whole thing, and I lost track.
To the greatest extent possible, I'd move away from putting more logic in the planner. I think it's fine for the planner to collect information (particularly planning information), but I'd try to pass that information directly into this function.


pkg/sql/explain_bundle.go, line 73 at r1 (raw file):

	err = db.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
		// Insert the bundle into system.statement_bundle_chunks.

while there's a single chunk, I'd modify the reading code to also deal with a single chunk


pkg/sql/explain_common.go, line 15 at r1 (raw file):

import "github.com/cockroachdb/cockroach/pkg/sql/opt/memo"

func (p *planner) FormatOptPlan(flags memo.ExprFmtFlags) string {

add a comment pls

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 @andreimatei, @dhartunian, @jordanlewis, and @nathanstilwell)


pkg/server/admin.go, line 1528 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

the "write" in the name seems confusing to me. I understand that we're technically writing something to a writer, but I'd name it more like getStatementBundle.

Done.


pkg/server/admin.go, line 1575 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

sup with this?

Done.


pkg/server/admin.go, line 1578 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

well, if this fails, it fails because there's no longer anybody to return the error to, right?

Removed.


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

Previously, andreimatei (Andrei Matei) wrote…

xxx

Done.


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

Previously, andreimatei (Andrei Matei) wrote…

put a comment here emphasizing that we don't return the rows for the inner query, but instead return a URL. And put a TODO to capture some rows and include them; I think being able to use this functionality through the admin ui to figure out what some requests are returning would be very cool.

Done.


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

Previously, andreimatei (Andrei Matei) wrote…

make a note pls that we're not getting here in case of query execution errors

Done.


pkg/sql/explain_bundle.go, line 30 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I looked briefly to see who integral the planner is to the whole thing, and I lost track.
To the greatest extent possible, I'd move away from putting more logic in the planner. I think it's fine for the planner to collect information (particularly planning information), but I'd try to pass that information directly into this function.

Good call, done.


pkg/sql/explain_bundle.go, line 73 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

while there's a single chunk, I'd modify the reading code to also deal with a single chunk

Why? We can switch to multiple chunks without worrying about whether there's another node with a version that can't read it.


pkg/sql/explain_common.go, line 15 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

add a comment pls

Done.

Copy link
Contributor

@andreimatei andreimatei left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dhartunian, @jordanlewis, @nathanstilwell, and @RaduBerinde)


pkg/sql/explain_bundle.go, line 73 at r1 (raw file):

Previously, RaduBerinde wrote…

Why? We can switch to multiple chunks without worrying about whether there's another node with a version that can't read it.

I see, ok

@jordanlewis
Copy link
Member

I don't want to bikeshed excessively but I'm concerned the name will give us an n+1 problem. I know this one is meant to be the final level of explain, but can we run it past product / docs / SE to see if the name is ok before we set it in stone? @awoods187

I also am still planning to do a review

Copy link
Contributor

@nathanstilwell nathanstilwell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

What about like EXPLAIN ANALYZE ALL? EXPLAIN ALL? EXPLAIN ANALYZE (ALL)? EXPLAIN ANALYZE (BUNDLE)?

Thing is that ANALYZE is well-known to definitely run the query. Not sure if people will expect something caleld BUNDLE to run the query.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @dhartunian, @jordanlewis, and @RaduBerinde)


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

Previously, RaduBerinde wrote…

Done.

what do you mean trimming?


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

	fingerprint := tree.AsStringWithFlags(plan.stmt.AST, tree.FmtHideConstants)
	statement := tree.AsString(plan.stmt.AST)
	description := "query support bundle XXX more info here"

XXX ^^


pkg/sql/explain_bundle_test.go, line 37 at r2 (raw file):

	defer srv.Stopper().Stop(context.Background())
	r := sqlutils.MakeSQLRunner(godb)
	rows := r.QueryStr(t, "EXPLAIN BUNDLE SELECT 1")

Can you replace this with a more sophisticated example that actually touches data? Just might catch more things.


pkg/sql/plan.go, line 361 at r2 (raw file):

func (p *planTop) formatOptPlan(flags memo.ExprFmtFlags) string {
	if p.mem == nil {
		return "<no optimizer plan>"

nit: I think this won't make sense to users... anything we can say in here to help? Maybe an explanation "no optimizer plan: see the xyz foo for more information", I don't know.

ctx, "statement-bundle-info-insert", txn,
sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
`INSERT INTO
system.statement_diagnostics(statement_fingerprint, statement, collected_at, bundle_chunks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RaduBerinde just to think through how the UI would show this, we would query the statement_diagnostics table by fingerprint, correct?

@RaduBerinde
Copy link
Member Author

Jordan, I chose this because I wanted it to be different than all the others because it operates differently and I wanted it to be easy to use. The ALL variants you listed are pretty cryptic. EXPLAIN ANALYZE (BUNDLE) I'd be ok with but it's annoyingly long. I'll let @awoods187 chip in too.

This is an initial version of EXPLAIN BUNDLE. Similar to EXPLAIN ANALYZE, it
runs the statement but throws away all resulting rows and instead returns a URL
to a zip bundle (which is served via a special HTTP endpoint); example:

```
root@127.249.95.44:37247/movr> EXPLAIN BUNDLE SELECT * FROM promo_codes;
                                  text
------------------------------------------------------------------------
  Download the bundle from:
    http://127.249.95.44:35445/_admin/v1/stmtbundle/535092672054657025
  or from the Admin UI (Advanced Debug -> Statement Diagnostics).
(1 row)
```

The bundle currently contains the statement and `EXPLAIN`, `EXPLAIN (OPT)`
plans. The plan is to add (in future changes) the `OPT, ENV` data, table stats,
`EXPLAIN ANALYZE` diagram, and traces.

This is not yet integrated with Andrei's query diagnostics work other than using
the same system.table. This will be addressed in future changes; the user will
be able to request a bundle+trace from the UI. Conversely, an EXPLAIN BUNDLE
will also fill out the `trace` field in the system table.

Note that the AdminUI will have an interface to list and retrieve collected
bundles.

Release note (sql change): EXPLAIN BUNDLE can be used to run a query and collect
execution information in a support bundle, which can be downloaded via the
AdminUI.
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 (waiting on @andreimatei, @dhartunian, and @jordanlewis)


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

Previously, jordanlewis (Jordan Lewis) wrote…

what do you mean trimming?

improved the comment


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

Previously, jordanlewis (Jordan Lewis) wrote…

XXX ^^

Done.


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

Previously, dhartunian (David Hartunian) wrote…

@RaduBerinde just to think through how the UI would show this, we would query the statement_diagnostics table by fingerprint, correct?

Correct


pkg/sql/explain_bundle_test.go, line 37 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Can you replace this with a more sophisticated example that actually touches data? Just might catch more things.

Done.


pkg/sql/plan.go, line 361 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: I think this won't make sense to users... anything we can say in here to help? Maybe an explanation "no optimizer plan: see the xyz foo for more information", I don't know.

Done.

@awoods187
Copy link
Contributor

I think we should talk this out on Monday. Do we expect appdevs to use this on their own? Will this only be for AppDevs escalating? Bundle captures the gist of what this does--but not its purpose. I'd love a name that focuses more on the outcome we expect to provide to the developer.

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

LGTM, we can always rename it after code freeze.

@RaduBerinde
Copy link
Member Author

Thanks! I'm merging and we'll connect on Monday to settle on a name.

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

7 participants