Skip to content

sql: add user_id column to system.role_options table #85845

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
RichardJCai:role_options_table_migration
Aug 13, 2022
Merged

sql: add user_id column to system.role_options table #85845
craig[bot] merged 2 commits intocockroachdb:masterfrom
RichardJCai:role_options_table_migration

Conversation

@RichardJCai
Copy link
Copy Markdown
Contributor

@RichardJCai RichardJCai commented Aug 9, 2022 •
edited
Loading

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
Copy Markdown
Member

cockroach-teamcity commented Aug 9, 2022

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 force-pushed the role_options_table_migration branch from 3c383c1 to d7a23b0 Compare August 10, 2022 19:16
@RichardJCai RichardJCai requested review from ajwerner and rafiss August 10, 2022 19:16
@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
Copy Markdown
Contributor Author

RichardJCai commented Aug 10, 2022

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
ajwerner
ajwerner approved these changes Aug 11, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

RichardJCai commented Aug 11, 2022 •
edited by petermattis
Loading

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
Copy Markdown
Contributor Author

RichardJCai commented Aug 11, 2022

Thanks for reviewing!

bors r=ajwerner

@RichardJCai RichardJCai force-pushed the role_options_table_migration branch from 3e0edf3 to b2d631b Compare August 12, 2022 03:11
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 12, 2022

Canceled.

@RichardJCai RichardJCai force-pushed the role_options_table_migration branch from b2d631b to 5f9429d Compare August 12, 2022 18:48
@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Aug 12, 2022

Trying again
bors r=ajwerner

@RichardJCai
sql: rename SystemUser table migration version keys to be more explicit
63a07d2 
Release note: None
@RichardJCai
sql: add user_id column to system.role_options table
d0ed951 
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
@RichardJCai RichardJCai force-pushed the role_options_table_migration branch from 5f9429d to d0ed951 Compare August 12, 2022 21:41
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 12, 2022

Canceled.

@RichardJCai
Copy link
Copy Markdown
Contributor Author

RichardJCai commented Aug 13, 2022

bors r=ajwerner

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 13, 2022

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 13, 2022

Build succeeded:

@craig craig bot merged commit dd59982 into cockroachdb:master Aug 13, 2022
@RichardJCai RichardJCai mentioned this pull request Aug 29, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rafiss rafiss Awaiting requested review from rafiss

@stevendanna stevendanna Awaiting requested review from stevendanna

1 more reviewer

@ajwerner ajwerner ajwerner approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@RichardJCai @cockroach-teamcity @ajwerner