Skip to content
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 role_id, member_id columns to system.role_members #85928

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Aug 10, 2022

sql: add role_id, member_id columns to system.role_members

Release note: None

Release justification: Adding is to system.role_members in same way
as system.users, last users table migration for this version.
We don't use the IDs to delete or lookup users yet.

Last system.users related table migration for 22.2

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RichardJCai RichardJCai force-pushed the role_members_table_migration branch 3 times, most recently from 30b3ab5 to ac98b30 Compare August 15, 2022 22:19
@RichardJCai RichardJCai changed the title Role members table migration sql: add role_id, member_id columns to system.role_members Aug 15, 2022
@RichardJCai RichardJCai marked this pull request as ready for review August 15, 2022 22:21
@RichardJCai RichardJCai requested review from a team August 15, 2022 22:21
@RichardJCai RichardJCai requested review from a team as code owners August 15, 2022 22:21
@RichardJCai RichardJCai requested review from rhu713, tbg and ajwerner and removed request for a team August 15, 2022 22:21
Release note: None

Release justification: Adding is to system.role_members in same way
as system.users, last users table migration for this version.
We don't use the IDs to delete or lookup users yet.
@tbg tbg removed their request for review August 16, 2022 12:53
@RichardJCai
Copy link
Contributor Author

Bump @ajwerner @rafiss, thoughts on merging this? Given that it's the same as the system.users and system.role_options ones, I figure it's fine. Also it's somewhat strange to have the other two with id columns and not this.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given nothing uses this, do we get benefit from having migrated all the tables? Is it correct that nothing uses it?

Reviewed 6 of 24 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rhu713)

@RichardJCai
Copy link
Contributor Author

Given nothing uses this, do we get benefit from having migrated all the tables? Is it correct that nothing uses it?

Reviewed 6 of 24 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rhu713)

Yeah we don't use it for reads right now.

@andyyang890
Copy link
Collaborator

Superseded by #91517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants