-
Notifications
You must be signed in to change notification settings - Fork 4k
sql: add index_usage_stats and datums_to_bytes to information_schema #156963
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 index_usage_stats and datums_to_bytes to information_schema #156963
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
a433c76 to
ffe82c9
Compare
14a8b98 to
18ebd8d
Compare
michae2
left a comment
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.
@michae2 reviewed 13 of 17 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons)
pkg/sql/crdb_internal.go line 242 at r2 (raw file):
// customer use in production for legacy reasons. var SupportedCRDBInternalTables = map[string]struct{}{ // LOCKED: Do not add to this list.
What does "LOCKED" mean?
pkg/sql/sem/builtins/builtins.go line 4751 at r2 (raw file):
}), "crdb_internal.datums_to_bytes": datumsToBytes,
Is there an allowlist similar to SupportedCRDBInternalTables but for builtin functions? If there were such a list, I would nominate crdb_internal.datums_to_bytes for it, because this function is used in the definition of every existing user-created hash-partitioned index. If we disallow usage of this function by default, I'm not sure applications will be able to write to or read from existing tables with hash-partitioned indexes.
(There are other functions I would also add to such a list, including:
crdb_internal.locality_valuecrdb_internal.request_statement_bundlecrdb_internal.clear_query_plan_cachecrdb_internal.clear_statement_hints_cachecrdb_internal.clear_table_stats_cachecrdb_internal.inject_hintcrdb_internal.reset_sql_statscrdb_internal.reset_index_usage_statscrdb_internal.set_vmodulecrdb_internal.log
all of which I believe are intended to be available to users.)
yuzefovich
left a comment
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 @angles-n-daemons and @michae2)
pkg/sql/crdb_internal.go line 241 at r2 (raw file):
// SupportedCRDBInternal are the crdb_internal tables that are "supported" for real // customer use in production for legacy reasons. var SupportedCRDBInternalTables = map[string]struct{}{
nit: it'd probably be worth adding an automatic check for this by having a unit test that asserts the contents against the hard-coded expectation - this way the CI at least will highlight that no new entries should be added.
angles-n-daemons
left a comment
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.
@angles-n-daemons reviewed 1 of 17 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
pkg/sql/sem/builtins/builtins.go line 4751 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
Is there an allowlist similar to
SupportedCRDBInternalTablesbut for builtin functions? If there were such a list, I would nominatecrdb_internal.datums_to_bytesfor it, because this function is used in the definition of every existing user-created hash-partitioned index. If we disallow usage of this function by default, I'm not sure applications will be able to write to or read from existing tables with hash-partitioned indexes.(There are other functions I would also add to such a list, including:
crdb_internal.locality_valuecrdb_internal.request_statement_bundlecrdb_internal.clear_query_plan_cachecrdb_internal.clear_statement_hints_cachecrdb_internal.clear_table_stats_cachecrdb_internal.inject_hintcrdb_internal.reset_sql_statscrdb_internal.reset_index_usage_statscrdb_internal.set_vmodulecrdb_internal.log
all of which I believe are intended to be available to users.)
Ah, yes we actually created it (and datums_to_bytes is a part of it). I'll check with my team, but so long as these are safe, we'll make sure they make it in too.
angles-n-daemons
left a comment
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 @michae2 and @yuzefovich)
pkg/sql/crdb_internal.go line 241 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it'd probably be worth adding an automatic check for this by having a unit test that asserts the contents against the hard-coded expectation - this way the CI at least will highlight that no new entries should be added.
that's a great idea, I'll add it to this PR
pkg/sql/crdb_internal.go line 242 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
What does "LOCKED" mean?
Ah, LOCKED just generally means we shouldn't be adding to this list of supported tables in the future, that we should instead be exposing information through the "Supported SQL Interface":
https://docs.google.com/document/d/1STbb8bljTzK_jXRIJrxtijWsPhGErdH1vZdunzPwXvs/edit?tab=t.0
michae2
left a comment
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 @angles-n-daemons and @yuzefovich)
pkg/sql/crdb_internal.go line 242 at r2 (raw file):
Previously, angles-n-daemons (Brian Dillmann) wrote…
Ah, LOCKED just generally means we shouldn't be adding to this list of supported tables in the future, that we should instead be exposing information through the "Supported SQL Interface":
https://docs.google.com/document/d/1STbb8bljTzK_jXRIJrxtijWsPhGErdH1vZdunzPwXvs/edit?tab=t.0
What happens if someone adds to this list?
michae2
left a comment
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 @angles-n-daemons and @yuzefovich)
pkg/sql/crdb_internal.go line 242 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
What happens if someone adds to this list?
(Asking because "LOCKED: Do not add to this list." is not a very clear explanation of why this list exists or why someone shouldn't add to it. What if someone discovers an existing crdb_internal table that a customer is reliant on? Can they add it to this list or not?)
michae2
left a comment
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.
Aside from my complaining about that comment, this
@michae2 reviewed 3 of 17 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @angles-n-daemons and @yuzefovich)
3d55494 to
c266da3
Compare
angles-n-daemons
left a comment
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 (and 1 stale) (waiting on @michae2 and @yuzefovich)
pkg/sql/crdb_internal.go line 242 at r2 (raw file):
Previously, michae2 (Michael Erickson) wrote…
(Asking because "LOCKED: Do not add to this list." is not a very clear explanation of why this list exists or why someone shouldn't add to it. What if someone discovers an existing crdb_internal table that a customer is reliant on? Can they add it to this list or not?)
The general idea is we only want to surface reviewed / clearly owned tables for end customers. In the future we want all customer accessible tables to be in the "stable sql interface (information_schema)" which adheres to stricter requirements than crdb_internal.
c266da3 to
d40d0ca
Compare
As part of the stable sql api effort, we're aiming to expose all "supported" internals to the information_schema. So that teams have an examplar, we here move index_usage_stats and datums_to_bytes both to information_schema, providing a foundation for future exposure of system internals. Fixes: cockroachdb#156677 Epic: CRDB-56107 Release note (sql change): Duplicates crdb_internal.index_usage_stats and crdb_internal.datums_to_bytes in information_schema (information_schema.crdb_index_usage_stats and information_schema.crdb_datums_to_bytes respectively).
d40d0ca to
470aff4
Compare
As part of the stable SQL API effort, expose the following builtin functions in information_schema to complement the existing crdb_internal versions: - crdb_internal.reset_index_usage_stats → information_schema.crdb_reset_index_usage_stats - crdb_internal.reset_sql_stats → information_schema.crdb_reset_sql_stats - crdb_internal.clear_query_plan_cache → information_schema.crdb_clear_query_plan_cache - crdb_internal.clear_table_stats_cache → information_schema.crdb_clear_table_stats_cache - crdb_internal.clear_statement_hints_cache → information_schema.crdb_clear_statement_hints_cache The implementations are shared between both schemas to ensure identical behavior. All existing test usages throughout the codebase have been updated to use the information_schema versions to demonstrate the new functions work correctly. This follows the pattern established in PR cockroachdb#156963 which ported index_usage_statistics and datums_to_bytes to information_schema. Epic: None Release note (sql change): Added information_schema versions of 5 builtin functions: crdb_reset_index_usage_stats, crdb_reset_sql_stats, crdb_clear_query_plan_cache, crdb_clear_table_stats_cache, and crdb_clear_statement_hints_cache. These complement the existing crdb_internal versions as part of the stable SQL API effort. Co-Authored-By: Claude <noreply@anthropic.com>
|
bors r+ |
156963: sql: add index_usage_stats and datums_to_bytes to information_schema r=angles-n-daemons a=angles-n-daemons As part of the stable sql api effort, we're aiming to expose all "supported" introspection tools in the information_schema. So that teams have an examplar, we here move index_usage_stats and datums_to_bytes both to information_schema, providing a foundation for future exposure of sql introspection tooling. Fixes: #156677 Epic: CRDB-56107 Release note (sql change): Duplicates crdb_internal.index_usage_stats and crdb_internal.datums_to_bytes in information_schema (information_schema.crdb_index_usage_stats and information_schema.crdb_datums_to_bytes respectively). 159004: logictest: deflake TestReadCommittedLogic_cluster_locks r=miraradeva a=arulajmani The test was trying to construct a scenario where there were two requests waiting on a single key. The test expected the order in which requests arrived to be deterministic, as the lock table releases requests in FIFO order. This isn't true with the async statement framework in logic tests. Here, we rework the test to not need two waiters on a single key; instead, we perform each test separately, resetting the test state in between runs. Closes #158769 Release note: None Co-authored-by: Brian Dillmann <brian.dillmann@cockroachlabs.com> Co-authored-by: Arul Ajmani <arulajmani@gmail.com>
As part of the stable SQL API effort, expose the following builtin functions in information_schema to complement the existing crdb_internal versions: - crdb_internal.reset_index_usage_stats → information_schema.crdb_reset_index_usage_stats - crdb_internal.reset_sql_stats → information_schema.crdb_reset_sql_stats - crdb_internal.clear_query_plan_cache → information_schema.crdb_clear_query_plan_cache - crdb_internal.clear_table_stats_cache → information_schema.crdb_clear_table_stats_cache - crdb_internal.clear_statement_hints_cache → information_schema.crdb_clear_statement_hints_cache The implementations are shared between both schemas to ensure identical behavior. All existing test usages throughout the codebase have been updated to use the information_schema versions to demonstrate the new functions work correctly. This follows the pattern established in PR cockroachdb#156963 which ported index_usage_statistics and datums_to_bytes to information_schema. Epic: None Release note (sql change): Added information_schema versions of 5 builtin functions: crdb_reset_index_usage_stats, crdb_reset_sql_stats, crdb_clear_query_plan_cache, crdb_clear_table_stats_cache, and crdb_clear_statement_hints_cache. These complement the existing crdb_internal versions as part of the stable SQL API effort. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
This PR was included in a batch that was canceled, it will be automatically retried |
As part of the stable SQL API effort, expose the following builtin functions in information_schema to complement the existing crdb_internal versions: - crdb_internal.reset_index_usage_stats → information_schema.crdb_reset_index_usage_stats - crdb_internal.reset_sql_stats → information_schema.crdb_reset_sql_stats - crdb_internal.clear_query_plan_cache → information_schema.crdb_clear_query_plan_cache - crdb_internal.clear_table_stats_cache → information_schema.crdb_clear_table_stats_cache - crdb_internal.clear_statement_hints_cache → information_schema.crdb_clear_statement_hints_cache The implementations are shared between both schemas to ensure identical behavior. All existing test usages throughout the codebase have been updated to use the information_schema versions to demonstrate the new functions work correctly. This follows the pattern established in PR cockroachdb#156963 which ported index_usage_statistics and datums_to_bytes to information_schema. Epic: None Release note (sql change): Added information_schema versions of 5 builtin functions: crdb_reset_index_usage_stats, crdb_reset_sql_stats, crdb_clear_query_plan_cache, crdb_clear_table_stats_cache, and crdb_clear_statement_hints_cache. These complement the existing crdb_internal versions as part of the stable SQL API effort. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
As part of the stable SQL API effort, expose the following builtin functions in information_schema to complement the existing crdb_internal versions: - crdb_internal.reset_index_usage_stats → information_schema.crdb_reset_index_usage_stats - crdb_internal.reset_sql_stats → information_schema.crdb_reset_sql_stats - crdb_internal.clear_query_plan_cache → information_schema.crdb_clear_query_plan_cache - crdb_internal.clear_table_stats_cache → information_schema.crdb_clear_table_stats_cache - crdb_internal.clear_statement_hints_cache → information_schema.crdb_clear_statement_hints_cache The implementations are shared between both schemas to ensure identical behavior. All existing test usages throughout the codebase have been updated to use the information_schema versions to demonstrate the new functions work correctly. This follows the pattern established in PR cockroachdb#156963 which ported index_usage_statistics and datums_to_bytes to information_schema. Epic: None Release note (sql change): Added information_schema versions of 5 builtin functions: crdb_reset_index_usage_stats, crdb_reset_sql_stats, crdb_clear_query_plan_cache, crdb_clear_table_stats_cache, and crdb_clear_statement_hints_cache. These complement the existing crdb_internal versions as part of the stable SQL API effort. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
As part of the stable sql api effort, we're aiming to expose all "supported" introspection tools in the information_schema. So that teams have an examplar, we here move index_usage_stats and datums_to_bytes both to information_schema, providing a foundation for future exposure of sql introspection tooling.
Fixes: #156677
Epic: CRDB-56107
Release note (sql change): Duplicates crdb_internal.index_usage_stats and crdb_internal.datums_to_bytes in information_schema (information_schema.crdb_index_usage_stats and
information_schema.crdb_datums_to_bytes respectively).