-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 user_id column to system.database_role_settings table #97251
sql: add user_id column to system.database_role_settings table #97251
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. |
ee13301
to
2325aff
Compare
2325aff
to
e7da559
Compare
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.
looks good overall! i just had one comment about using a STORING index
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890)
pkg/upgrade/upgrades/database_role_settings_table_user_id_migration.go
line 31 at r1 (raw file):
const createUniqueIndexOnDatabaseIDAndRoleIDOnDatabaseRoleSettingsTableStmt = ` CREATE UNIQUE INDEX IF NOT EXISTS database_role_settings_database_id_role_id_key ON system.database_role_settings (database_id, role_id)
similar to system.privileges, i think this would benefit from having STORING (settings)
on it. (if you look at the query where the table is accessed, it always needs to fetch the settings column)
e7da559
to
ce3b560
Compare
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 @rafiss)
pkg/upgrade/upgrades/database_role_settings_table_user_id_migration.go
line 31 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
similar to system.privileges, i think this would benefit from having
STORING (settings)
on it. (if you look at the query where the table is accessed, it always needs to fetch the settings column)
Done.
This patch adds a new `role_id` column to the `system.database_role_settings` table, which corresponds to the existing `role_name` column. Migrations are also added to alter and backfill the table in older clusters. Release note: None
ce3b560
to
5225a32
Compare
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.
lgtm!
Reviewable status: complete! 0 of 0 LGTMs obtained
TFTR! bors r=rafiss |
Build succeeded: |
This patch adds a new
role_id
column to thesystem.database_role_settings
table, which corresponds to the existing
role_name
column. Migrations arealso added to alter and backfill the table in older clusters.
Part of #87079
Release note: None