sqlstats: add idle_lat to node_statement_statistics#92281
sqlstats: add idle_lat to node_statement_statistics#92281craig[bot] merged 1 commit intocockroachdb:masterfrom matthewtodd:idle_lat-node_statement_statistics
Conversation
|
This change is |
xinhaoz
left a comment
There was a problem hiding this comment.
Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)
pkg/sql/crdb_internal.go line 1186 at r1 (raw file):
rows_var FLOAT NOT NULL, idle_lat_avg FLOAT NOT NULL, idle_lat_var FLOAT NOT NULL,
Just wondering, do we have any sort of precedence of adding new columns to the end, to preserve the order of columns in things like SELECT * ?
matthewtodd
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 1186 at r1 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Just wondering, do we have any sort of precedence of adding new columns to the end, to preserve the order of columns in things like SELECT * ?
Good question! I have no idea -- I'll ask. (And maybe it's okay here, since we're non-committal about the stability of this table?)
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 1186 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Good question! I have no idea -- I'll ask. (And maybe it's okay here, since we're non-committal about the stability of this table?)
separately, just wondering: are these new columns also going to end up being flushed to system.statement_statistics and appear in the crdb_internal.statement_statistics view?
i think in the case of tables that are actually "internal only" adding columns in the middle is fine. but for the externally-used tables, they should be added at the end. so if these will be at the end of crdb_internal.statement_statistics, then maybe they should be at the end here too. up to you, i don't feel strongly
matthewtodd
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go line 1186 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
separately, just wondering: are these new columns also going to end up being flushed to
system.statement_statisticsand appear in thecrdb_internal.statement_statisticsview?i think in the case of tables that are actually "internal only" adding columns in the middle is fine. but for the externally-used tables, they should be added at the end. so if these will be at the end of
crdb_internal.statement_statistics, then maybe they should be at the end here too. up to you, i don't feel strongly
Thank you both. It turns out this is the only table with explicit columns for these values: crdb_internal.statement_statistics and system.statement_statistics put them into a json blob in their statistics columns instead, so we're all good!
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Part of #86667.
For parity with our existing statement-level work in #91098.
(No release note here since
node_statement_statisticsis not one of the "Use in production"crdb_internaltables.)Release note: None