sql: add execution stats to node_statement_statistics #62006
Conversation
|
This change is |
|
I did not run any tests locally, so I'm expecting CI to be red. Will fix and repush once that happens. |
|
I'll also update |
|
OK, should be ready for a review now. |
There was a problem hiding this comment.
A couple of nits which you should feel free to ignore.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @asubiotto, @Azhng, and @maryliag)
pkg/sql/crdb_internal.go, line 861 at r1 (raw file):
rows_read_avg FLOAT NOT NULL, rows_read_var FLOAT NOT NULL, network_bytes_avg FLOAT,
super nit: it might be nicer to put two "network" fields next each other as well as two "memory usage" fields.
pkg/sql/crdb_internal.go, line 964 at r1 (raw file):
tree.NewDFloat(tree.DFloat(s.mu.data.RowsRead.Mean)), tree.NewDFloat(tree.DFloat(s.mu.data.RowsRead.GetVariance(s.mu.data.Count))), execStatAvg(s.mu.data.ExecStats.Count, s.mu.data.ExecStats.NetworkBytes),
super nit: it might be helpful to put an inlined comment for all of these with the column name, e.g. this line would be // network_bytes_avg. We seem to follow such pattern for some virtual tables, and for this one in particular it would beneficial since it is very wide.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go, line 861 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
super nit: it might be nicer to put two "network" fields next each other as well as two "memory usage" fields.
Done.
pkg/sql/crdb_internal.go, line 964 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
super nit: it might be helpful to put an inlined comment for all of these with the column name, e.g. this line would be
// network_bytes_avg. We seem to follow such pattern for some virtual tables, and for this one in particular it would beneficial since it is very wide.
Done.
There was a problem hiding this comment.
Thanks!
Reviewed 4 of 4 files at r2.
Reviewable status:
pkg/sql/crdb_internal.go, line 869 at r2 (raw file):
max_disk_usage_avg FLOAT, max_disk_usage_var FLOAT, contention_time_avg FLOAT,
super nit: seems like a dangling space.
Release justification: low-risk, high-benefit change to existing functionality
since this commit exposes pre-existing execution stats through the SQL shell.
Release note (sql change): sampled execution stats are now available through
node_{statement,transaction}_statistics.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go, line 869 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
super nit: seems like a dangling space.
Done.
|
TFTR bors r=yuzefovich |
|
Build succeeded: |
Release justification: low-risk, high-benefit change to existing functionality
since this commit exposes pre-existing execution stats through the SQL shell.
Release note (sql change): sampled execution stats are now available through
node_statement_statistics.
Closes #61637
Putting @yuzefovich as primary reviewer but @maryliag and @Azhng, please take a look and let me know if you have any questions.