-
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 role_id and member_id columns to system.role_members table #91517
Conversation
e3e2f9f
to
11cb42f
Compare
7fd2d22
to
b3a1638
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.
this is looking good! i just had small comments
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890)
pkg/upgrade/upgrades/role_members_ids_migration.go
line 87 at r1 (raw file):
const backfillIDColumnsRoleMembersStmt = ` UPDATE system.role_members
I'm mildly worried about the cross join on system.users, since that could potentially lead to a lot of memory usage while executing the query. i wonder if it would be easier to reason about if we did with two separate queries:
UPDATE system.role_members
SET role_id = u.user_id
FROM system.users u
WHERE role_id IS NULL and role = u.username
LIMIT 1;
UPDATE system.role_members
SET member_id = u.user_id
FROM system.users u
WHERE member_id IS NULL and member = u.username
pkg/upgrade/upgrades/role_members_ids_migration.go
line 97 at r1 (raw file):
ctx context.Context, _ clusterversion.ClusterVersion, d upgrade.TenantDeps, _ *jobs.Job, ) error { row, err := d.InternalExecutor.QueryRowEx(ctx, `get-num-null-role-ids-system-role-members`, nil,
nit: we don't need this query - we can just keep running the update query until it returns a "rows affected" of 0
b3a1638
to
0cfe823
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/role_members_ids_migration.go
line 87 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
I'm mildly worried about the cross join on system.users, since that could potentially lead to a lot of memory usage while executing the query. i wonder if it would be easier to reason about if we did with two separate queries:
UPDATE system.role_members SET role_id = u.user_id FROM system.users u WHERE role_id IS NULL and role = u.username LIMIT 1; UPDATE system.role_members SET member_id = u.user_id FROM system.users u WHERE member_id IS NULL and member = u.username
Done.
0cfe823
to
80f7816
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/role_members_ids_migration.go
line 97 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: we don't need this query - we can just keep running the update query until it returns a "rows affected" of 0
Fixed.
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.
Left some comments for usages of internal executor somehow my comments are gone! Leaving them again
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 @andyyang890 and @rafiss)
pkg/sql/grant_role.go
line 170 at r3 (raw file):
func (n *GrantRoleNode) startExec(params runParams) error { opName := "grant-role"
Could you wrap all contents in this function with
params.p.WithInternalExecutor(params.ctx, func(ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor) error{<content>}
, and change the params.p.ExecCfg().InternalExecutor
or params.extendedEvalCtx.ExecCfg.InternalExecutor
to ie
? Thank you!
pkg/upgrade/upgrades/role_members_ids_migration.go
line 99 at r3 (raw file):
for _, colName := range []string{"role_id", "member_id"} { for { rowsAffected, err := d.InternalExecutor.ExecEx(ctx, "backfill-id-columns-system-role-members", nil,
Could you change it to
ie := d.InternalExecutorFactory.MakeInternalExecutorWithoutTxn()
for. ... {
for ... {
rowsAffected, err := ie.ExecEx(ctx, "backfill-id-columns-system-role-members", nil /* txn */,
sessiondata.NodeUserSessionDataOverride,
fmt.Sprintf(backfillIDColumnsRoleMembersStmt, colName),
)
}
}
We'd like to advocate using MakeInternalExecutorWithoutTxn
for internal executor that run queries with nil
txn.
Previously, ZhouXing19 (Jane Xing) wrote…
Sorry, |
c9ac159
to
a91d4b7
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 and @ZhouXing19)
pkg/sql/grant_role.go
line 170 at r3 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
Could you wrap all contents in this function with
params.p.WithInternalExecutor(params.ctx, func(ctx context.Context, txn *kv.Txn, ie sqlutil.InternalExecutor) error{<content>}
, and change theparams.p.ExecCfg().InternalExecutor
orparams.extendedEvalCtx.ExecCfg.InternalExecutor
toie
? Thank you!
Done.
pkg/upgrade/upgrades/role_members_ids_migration.go
line 99 at r3 (raw file):
Previously, ZhouXing19 (Jane Xing) wrote…
Sorry,
d.InternalExecutor.ExecEx
should beie.ExecEx
in the comment above.
Done.
d693359
to
83d507a
Compare
TFTR! bors r=rafiss |
Build failed (retrying...): |
Build failed: |
6d90b41
to
8629d41
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 @andyyang890, @benbardin, @rafiss, and @ZhouXing19)
pkg/clusterversion/cockroach_versions.go
line 610 at r12 (raw file):
}, { Key: V23_1RoleMembersTableHasIDColumns,
is there a reason to have 3 distinct versions and 3 upgrades instead of a single one?
pkg/upgrade/upgrades/permanent_upgrades.go
line 36 at r12 (raw file):
ctx context.Context, _ clusterversion.ClusterVersion, deps upgrade.TenantDeps, ) error { ieNotBoundToTxn := deps.InternalExecutorFactory.MakeInternalExecutorWithoutTxn()
pls make this change in a different commit, if it's needed.
Is it need? I see other uses of TenantDeps.InternalExecutor
. If that's no good, consider getting rid of all of them at once.
pkg/upgrade/upgrades/permanent_upgrades.go
line 62 at r12 (raw file):
_, err = ieNotBoundToTxn.Exec( ctx, "addRootToAdminRole", nil /* txn */, upsertMembership, username.AdminRole, username.RootUser, username.AdminRoleID, username.RootUserID) if err != nil {
either a blind return, as it was before, or return nil
at the end if you're handling the error
8629d41
to
cd25722
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 @andreimatei, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/clusterversion/cockroach_versions.go
line 610 at r12 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
is there a reason to have 3 distinct versions and 3 upgrades instead of a single one?
I based this PR off of #81457 and I believe @knz left a comment saying this should be done in three versions here #81457 (review).
pkg/upgrade/upgrades/permanent_upgrades.go
line 36 at r12 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
pls make this change in a different commit, if it's needed.
Is it need? I see other uses ofTenantDeps.InternalExecutor
. If that's no good, consider getting rid of all of them at once.
Ok I moved it out. I'll defer to @ZhouXing19 to comment on why it's needed.
pkg/upgrade/upgrades/permanent_upgrades.go
line 62 at r12 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
either a blind return, as it was before, or
return nil
at the end if you're handling the error
Done.
We ultimately want to get rid of |
Interestingly, removing the addition of bors r=rafiss |
Build failed (retrying...): |
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 @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/clusterversion/cockroach_versions.go
line 610 at r12 (raw file):
Previously, andyyang890 (Andy Yang) wrote…
I based this PR off of #81457 and I believe @knz left a comment saying this should be done in three versions here #81457 (review).
I believe Raphael said two versions, not 3 ("version 1" in that list is pre-existing). Like, why do the indexes need to be added in a separate upgrade?
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 @andyyang890, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/upgrade/upgrades/permanent_upgrades.go
line 56 at r13 (raw file):
// Upsert the role membership into the table. We intentionally override any existing entry. const upsertMembership = ` UPSERT INTO system.role_members ("role", "member", "isAdmin", role_id, member_id) VALUES ($1, $2, true, $3, $4)
Wait, is this right? This can run in a 22.2 cluster, before the columns have been added.
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 @andyyang890, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/upgrade/upgrades/role_members_ids_migration.go
line 42 at r13 (raw file):
const addIndexOnRoleIDToRoleMembersStmt = ` CREATE INDEX role_members_role_id_idx ON system.role_members (role_id ASC)
I think these all need if not exists
, right? Upgrades need to be idempotent.
bors r- |
Canceled. |
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 @andreimatei, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/upgrade/upgrades/role_members_ids_migration.go
line 42 at r13 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I think these all need
if not exists
, right? Upgrades need to be idempotent.
Idempotence is already provided by hasIndex
, which is the schemaExistsFn
in the operation
structs defined below. None of the other upgrades seem to use IF NOT EXISTS
with CREATE INDEX
, but I can add it anyway to be extra safe.
cd25722
to
746e7d7
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 @andreimatei, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/clusterversion/cockroach_versions.go
line 610 at r12 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
I believe Raphael said two versions, not 3 ("version 1" in that list is pre-existing). Like, why do the indexes need to be added in a separate upgrade?
Ok I've moved the adding of the new indexes into the same upgrade as the columns being added.
pkg/upgrade/upgrades/role_members_ids_migration.go
line 42 at r13 (raw file):
Previously, andyyang890 (Andy Yang) wrote…
Idempotence is already provided by
hasIndex
, which is theschemaExistsFn
in theoperation
structs defined below. None of the other upgrades seem to useIF NOT EXISTS
withCREATE INDEX
, but I can add it anyway to be extra safe.
Added now.
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 @andreimatei, @benbardin, @knz, @rafiss, and @ZhouXing19)
pkg/upgrade/upgrades/permanent_upgrades.go
line 56 at r13 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Wait, is this right? This can run in a 22.2 cluster, before the columns have been added.
Discussed offline and this should be fixed by #93071. Will wait for that PR to be merged and then rebase before attempting to merge this PR.
This patch adds user ID columns to the system.role_members table. The new `role_id` column corresponds to the existing `role` column and the new `member_id` column corresponds to the existing `member` column. Migrations are also added to alter and backfill the table in older clusters. Release note: None
746e7d7
to
7adcd34
Compare
bors r=rafiss |
Build succeeded: |
This patch adds user ID columns to the system.role_members table.
The new
role_id
column corresponds to the existingrole
columnand the new
member_id
column corresponds to the existingmember
column. Migrations are also added to alter and backfill the table
in older clusters.
Part of #87079
Release note: None