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 user_id column to system.role_options table #85845

Merged
merged 2 commits into from
Aug 13, 2022

Conversation

RichardJCai
Copy link
Contributor

@RichardJCai RichardJCai commented Aug 9, 2022

The system.role_options user_id column is populated after
the system.users user_id column is populated in the case of the
migration.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RichardJCai RichardJCai force-pushed the role_options_table_migration branch 3 times, most recently from b73df3f to 3c383c1 Compare August 10, 2022 19:13
@RichardJCai RichardJCai changed the title WIP: role options table migration sql: add user_id column to system.role_options table Aug 10, 2022
@RichardJCai RichardJCai marked this pull request as ready for review August 10, 2022 19:16
@RichardJCai RichardJCai requested review from a team August 10, 2022 19:16
@RichardJCai RichardJCai requested a review from a team as a code owner August 10, 2022 19:16
@RichardJCai RichardJCai force-pushed the role_options_table_migration branch 2 times, most recently from fa1464c to df6cc1f Compare August 10, 2022 19:19
@RichardJCai
Copy link
Contributor Author

This PR borrows a lot from #81457 - the PR to add the user_id column to system.users but should be slightly more straightforward since we no longer have to generate IDs, they should exist in system.users already

@RichardJCai RichardJCai force-pushed the role_options_table_migration branch 2 times, most recently from 2b46c74 to e635a2a Compare August 11, 2022 15:30
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.

This seems fine. :lgtm:

Reviewed 4 of 4 files at r1, 30 of 57 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, @RichardJCai, and @stevendanna)


pkg/bench/rttanalysis/testdata/benchmark_expectations line 32 at r3 (raw file):

14,CreateRole/create_role_with_1_option
17,CreateRole/create_role_with_2_options
20,CreateRole/create_role_with_3_options

how come these are going up?

Code quote:

14,CreateRole/create_role_with_1_option
17,CreateRole/create_role_with_2_options
20,CreateRole/create_role_with_3_options
15,CreateRole/create_role_with_no_options

@RichardJCai
Copy link
Contributor Author

RichardJCai commented Aug 11, 2022

This seems fine. :lgtm:

Reviewed 4 of 4 files at r1, 30 of 57 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, @RichardJCai, and @stevendanna)

pkg/bench/rttanalysis/testdata/benchmark_expectations line 32 at r3 (raw file):

14,CreateRole/create_role_with_1_option
17,CreateRole/create_role_with_2_options
20,CreateRole/create_role_with_3_options

how come these are going up?

Code quote:

14,CreateRole/create_role_with_1_option
17,CreateRole/create_role_with_2_options
20,CreateRole/create_role_with_3_options
15,CreateRole/create_role_with_no_options

Additional round trip from looking up user_id from username, I'm gonna add caching for this.

@RichardJCai RichardJCai force-pushed the role_options_table_migration branch 2 times, most recently from e8d360f to 3e0edf3 Compare August 11, 2022 19:24
@RichardJCai
Copy link
Contributor Author

Thanks for reviewing!

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Canceled.

@RichardJCai
Copy link
Contributor Author

Trying again
bors r=ajwerner

The system.role_options user_id column is populated after
the system.users user_id column is populated in the case of the
migration.

Release note: None
@craig
Copy link
Contributor

craig bot commented Aug 12, 2022

Canceled.

@RichardJCai
Copy link
Contributor Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 13, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 13, 2022

Build succeeded:

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.

3 participants