sql: add crdb_internal views for system statistics tables#98261
sql: add crdb_internal views for system statistics tables#98261craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
j82w
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal line 212 at r1 (raw file):
1 can_view_node_info false 1 can_view_tsdb_metrics false 5 can_admin_split false
Are these changes expected? They don't seem to be related to this PR.
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 25 at r1 (raw file):
SHOW TABLES FROM crdb_internal ---- crdb_internal active_range_feeds table admin NULL NULL
Can we add a logic test to verify the view can be read with view activity permissions, and that the view matches the system table?
af56642 to
8db1e93
Compare
ericharmeling
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 25 at r1 (raw file):
Can we add a logic test to verify the view can be read with view activity permissions
I don't think the user needs any permissions to view the crdb_internal views... I saw this locally, and the logic test I just added verifies this.
that the view matches the system table?
Done.
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal line 212 at r1 (raw file):
Previously, j82w (Jake) wrote…
Are these changes expected? They don't seem to be related to this PR.
I agree - this seems a bit off.
ericharmeling
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/crdb_internal line 1262 at r2 (raw file):
query TTTITTT colnames SELECT * FROM crdb_internal.transaction_statistics_persisted WHERE NOT EXISTS (SELECT * FROM system.transaction_statistics);
I'm actually going to update these queries a bit and push with some test rewrites...
8db1e93 to
752f587
Compare
752f587 to
96cce4d
Compare
|
Can you please create a new file in |
96cce4d to
03f8947
Compare
Sure thing! These should just check |
03f8947 to
96d5f50
Compare
|
Yes please! |
Okay! I've added the We are planning to add some indexes to the underlying |
Thanks! nit: can you please rename to |
|
Isn't this a bug? Shouldn't it at least require ViewActivity Or ViewActivityRedacted Role like most of the virtual tables? |
|
Previously, j82w (Jake) wrote…
Isn't this a bug? Shouldn't it at least require ViewActivity Or ViewActivityRedacted Role like most of the virtual tables? |
ericharmeling
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant line 25 at r1 (raw file):
Previously, j82w (Jake) wrote…
Isn't this a bug? Shouldn't it at least require ViewActivity Or ViewActivityRedacted Role like most of the virtual tables?
It looks like the default grant/grantee for all crdb_internal virtual tables and views is SELECT/public, so unless we check the role on the user when generating the vtables/views, anyone can select any crdb_internal vtable/view.
For example:
(as root)
root@127.0.0.1:26257/movr> create user eric;
CREATE ROLE
root@127.0.0.1:26257/movr> show roles;
username | options | member_of
-----------+---------+------------
admin | | {}
eric | | {}
root | | {admin}
(3 rows)
root@127.0.0.1:26257/movr> show grants on crdb_internal.transaction_contention_events;
database_name | schema_name | table_name | grantee | privilege_type | is_grantable
----------------+---------------+-------------------------------+---------+----------------+---------------
movr | crdb_internal | transaction_contention_events | public | SELECT | f
(1 row)
root@127.0.0.1:26257/movr> show grants on crdb_internal.statement_statistics;
database_name | schema_name | table_name | grantee | privilege_type | is_grantable
----------------+---------------+----------------------+---------+----------------+---------------
movr | crdb_internal | statement_statistics | public | SELECT | f
(1 row)
(as non-admin with no permissions)
eric@127.0.0.1:26257/defaultdb> select * from crdb_internal.transaction_contention_events;
ERROR: user eric does not have VIEWACTIVITY or VIEWACTIVITYREDACTED privilege
eric@127.0.0.1:26257/defaultdb> select fingerprint_id from crdb_internal.statement_statistics limit 3;
fingerprint_id
----------------------
\xef388c6ce139b396
\x6906bca0125df305
\x120ef13c86e9acc9
(3 rows)
We check for VIEWACTIVITY/VIEWACTIVITYREDACTED for crdb_internal.transaction_contention_events here:
cockroach/pkg/sql/crdb_internal.go
Line 6592 in 5207de8
But we never do for crdb_internal.statement_statistics. And we don't check any permissions for any crdb_internal views. Perhaps we should? @maryliag
We should also consult Sessions on this. @rafiss
96d5f50 to
c16d1b3
Compare
Done! |
maryliag
left a comment
There was a problem hiding this comment.
Nice!
Just one question on the tests, otherwise
Reviewed 5 of 6 files at r1, 6 of 11 files at r3, 1 of 3 files at r4, 6 of 6 files at r6, all commit messages.
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/sql_statistics_persisted line 19 at r6 (raw file):
indexes_usage FROM system.statement_statistics
don't you wanna test a select on the new views too?
This commit adds two new crdb_internal views: - crdb_internal.statement_statistics_persisted, which surfaces the system.statement_statistics table - crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table Epic: none Release note: Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.
c16d1b3 to
4557dd0
Compare
ericharmeling
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/sql/opt/exec/execbuilder/testdata/sql_statistics_persisted line 19 at r6 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
don't you wanna test a select on the new views too?
Done.
|
blathers backport 22.2 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 4557dd0 to blathers/backport-release-22.2-98261: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Backport of commit 4557dd0 (cockroachdb#98261). This commit adds two new crdb_internal views: - crdb_internal.statement_statistics_persisted, which surfaces the system.statement_statistics table - crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table Epic: none Release note (sql change): Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.
Backport of commit 4557dd0 (cockroachdb#98261). This commit adds two new crdb_internal views: - crdb_internal.statement_statistics_persisted, which surfaces the system.statement_statistics table - crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table Epic: none Release note: Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.
Backport of commit 4557dd0 (cockroachdb#98261). This commit adds two new crdb_internal views: - crdb_internal.statement_statistics_persisted, which surfaces the system.statement_statistics table - crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table Epic: none Release note: Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.
Backport of commit 4557dd0 (cockroachdb#98261). This commit adds two new crdb_internal views: - crdb_internal.statement_statistics_persisted, which surfaces the system.statement_statistics table - crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table Epic: none Release note: Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.
This commit adds two new crdb_internal views:
Example output from after flush:
Epic: none
Release note: Added two views to the crdb_internal catalog: crdb_internal.statement_statistics_persisted, which surfaces data in the persisted system.statement_statistics table, and crdb_internal.transaction_statistics_persisted, which surfaces the system.transaction_statistics table.