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 query level stats to EXPLAIN ANALYZE #60641
Conversation
I am unsure of the max-memory and network usage stats. Do we think these are currently robust enough to be useful? I could hide them behind the |
Could you post some examples for the issue?
I'd vote in favor of adding them to Explain Analyze. |
Example:
Any thoughts on whether any of it should be VERBOSE-only? |
I love it! I'd vote to keep them in EXPLAIN ANALYZE and not in a verbose only mode. |
1ac6081
to
e4ea2ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in agreement with Andy. It might not be useful in some cases but I think it's better to show more than less information (users might not even know there is a verbose mode) and iterate from there if we get negative feedback.
Reviewed 13 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde)
pkg/sql/explain_plan.go, line 174 at r1 (raw file):
if explainPlan == nil { return errors.New("no plan")
Is there enough context to understand this error?
pkg/sql/instrumentation.go, line 262 at r1 (raw file):
stmtStats.mu.Unlock() } txnStats.Accumulate(queryLevelStats)
I'm not sure we want to call Accumulate
outside the block. Might lead to cases where stats don't add up in the DB console (i.e. one statement in a txn has nil stmt stats but what was collected is reflected in the txn stats).
pkg/sql/logictest/testdata/logic_test/explain_analyze, line 39 at r1 (raw file):
distribution: <hidden> vectorized: <hidden> rows read from KV: 3 (24 B)
Are the bytes values deterministic?
This change adds top-level query stats to the EXPLAIN ANALYZE output. Release note (sql change): EXPLAIN ANALYZE now shows more top-level query statistics.
e4ea2ec
to
446bc02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
pkg/sql/explain_plan.go, line 174 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Is there enough context to understand this error?
We are checking at a higher level, this is a "just in case" check (I ran into a NPE while working on this change). In general, no plan means that there was an error during planning. Changed to an AssertionFailedf
.
pkg/sql/instrumentation.go, line 262 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I'm not sure we want to call
Accumulate
outside the block. Might lead to cases where stats don't add up in the DB console (i.e. one statement in a txn has nil stmt stats but what was collected is reflected in the txn stats).
Done.
pkg/sql/logictest/testdata/logic_test/explain_analyze, line 39 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Are the bytes values deterministic?
Yeah, 8 per row
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)
Build failed (retrying...): |
Build succeeded: |
This change adds top-level query stats to the EXPLAIN ANALYZE output.
Release note (sql change): EXPLAIN ANALYZE now shows more top-level
query statistics.