-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: add contended tables, indexes, and keys views #66370
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 contended tables, indexes, and keys views #66370
Conversation
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.
Excellent work!
I'm, at a high level, wondering about 2 things (both product level):
- Internal table architecture-wise, do we prefer our join keys to be by-name (in which case any joins must be accomplished by including all 3 of the names in a join) or by-id (in which case joins can be accomplished by just the table id, which are cluster-wide unique?)
- Do we need to redact the contention keys? The answer to this question might depend on the level of access that we provide to this table. It seems like a data leak waiting to happen unless we also test the querying user's
SELECT
capability on the tables.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)
pkg/sql/crdb_internal.go, line 1753 at r1 (raw file):
database_name, schema_name, name, sum(num_contention_events) FROM [
Interesting use of the cockroachdb-specific [] syntax! I'm curious, how did you come across this? In general we prefer the SQL-standard () subquery syntax, which is going to be better supported optimization-wise, unless you found a reason not to use it here?
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 agree we should have redaction depending on the user's level of access to the table. Also, should we have some explanation or more details about what the value means? I don't think the user will necessarily knows what each value of the key means and even if you look for documentation I could only find https://www.cockroachlabs.com/docs/stable/functions-and-operators.html that simply states "This function is used only by CockroachDB’s developers for testing purposes."
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)
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.
Thank you both for the review!
For 1., I could use a little more context. I've asked in our #sql-observability-team to see more of what you mean, @jordanlewis.
For 2., AFAICT this change doesn't expose anything that isn't already available in crdb_internal.cluster_contention_events
, which is already gated on a VIEWACTIVITY permission. And I assume a view doesn't work if the user can't select from its underlying tables, yes?
Alternatively, I've gone looking for column-specific permissions but not yet found anything. If those exist, could you point me in the right direction?
Prettifying keys was per @kevin-v-ngo's request, acknowledging that it's not yet an ideal answer. LMK if you think we should just revert to binary keys here for now, and I can do that easily.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/crdb_internal.go, line 1753 at r1 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
Interesting use of the cockroachdb-specific [] syntax! I'm curious, how did you come across this? In general we prefer the SQL-standard () subquery syntax, which is going to be better supported optimization-wise, unless you found a reason not to use it here?
Oops! Not sure where I saw this, will fix. Good catch!
These support developers in interpreting the existing contention information in cluster_contention_events. Resolves: #61507 See also: #57114, #61625 Release note (sql change): Three new views were added to the crdb_internal schema to support developers investigating contention events, cluster_contended_{tables, indexes, keys}.
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 think we're all set to merge this now! I've fixed my unintentional usage of cockroach-specific syntax, @kevin-v-ngo is following up on the key redaction issue, and @jordanlewis mentioned in our 1:1 this morning that his comments about the shape of these tables/views were more intended to raise our team's overall awareness (leading to Kevin's draft doc, awesome, Kevin!) than to block landing these changes as they are. Could I get an LGTM?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis)
pkg/sql/crdb_internal.go, line 1753 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Oops! Not sure where I saw this, will fix. Good catch!
Done.
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! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
bors r+ |
Build succeeded: |
These support developers in interpreting the existing contention information in cluster_contention_events.
Resolves: #61507
See also: #57114, #61625
Release note (sql change): Three new views were added to the crdb_internal schema to support developers investigating contention events, cluster_contended_{tables, indexes, keys}.