sql: add role_id and member_id columns to system.role_members table by andyyang890 · Pull Request #91517 · cockroachdb/cockroach

andyyang890 · 2022-11-08T16:09:17Z

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.

Part of #87079

Release note: None

cockroach-teamcity · 2022-11-08T16:10:04Z

This change is

rafiss

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

andyyang890

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.

andyyang890

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.

ZhouXing19

~~Left some comments for usages of internal executor~~ somehow my comments are gone! Leaving them again

ZhouXing19

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.

ZhouXing19 · 2022-11-17T20:53:34Z

pkg/upgrade/upgrades/role_members_ids_migration.go line 99 at r3 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Could you change it to

ie := d.InternalExecutorFactory.MakeInternalExecutorWithoutTxn()
for. ... {
  for ... {
    rowsAffected, err := d.InternalExecutor.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.

Sorry, d.InternalExecutor.ExecEx should be ie.ExecEx in the comment above.

andyyang890

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 the params.p.ExecCfg().InternalExecutor or params.extendedEvalCtx.ExecCfg.InternalExecutor to ie? 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 be ie.ExecEx in the comment above.

Done.

andyyang890 · 2022-12-01T22:16:44Z

TFTR!

bors r=rafiss

craig · 2022-12-02T00:14:51Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2022-12-02T02:36:42Z

Build failed:

Bazel Essential CI (Cockroach)

andreimatei

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

andyyang890

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 of TenantDeps.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.

ZhouXing19 · 2022-12-02T18:24:42Z

pkg/upgrade/upgrades/permanent_upgrades.go line 36 at r12 (raw file):

Is it need? I see other uses of TenantDeps.InternalExecutor. If that's no good, consider getting rid of all of them at once.

We ultimately want to get rid of TenantDeps.InternalExecutor, as stated in #91004.
I don't think we need to get rid of them all in this PR -- there are 27 direct usages of TenantDeps.InternalExecutor and some of them can involve more complicated changes. I've made #92938 to track it and will work on it shortly.

andyyang890 · 2022-12-02T19:08:24Z

Interestingly, removing the addition of ieNotBoundToTxn in pkg/upgrade/upgrades/permanent_upgrades.go seems to have fixed the issue of the acceptance/version-upgrade roachtest hanging. Will try merging again now!

bors r=rafiss

craig · 2022-12-02T19:16:40Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

andreimatei

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?

andreimatei

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.

andreimatei

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.

andyyang890 · 2022-12-02T19:42:32Z

bors r-

craig · 2022-12-02T19:42:34Z

Canceled.

andyyang890

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.

andyyang890

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 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.

Added now.

andyyang890

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

andyyang890 · 2022-12-09T02:01:50Z

bors r=rafiss

craig · 2022-12-09T02:55:09Z

Build succeeded:

Bazel Essential CI (Cockroach)

andyyang890 force-pushed the migrate_role_members branch 2 times, most recently from e3e2f9f to 11cb42f Compare November 10, 2022 20:51

andyyang890 mentioned this pull request Nov 15, 2022

sql: add role_id, member_id columns to system.role_members #85928

Closed

andyyang890 force-pushed the migrate_role_members branch 3 times, most recently from 7fd2d22 to b3a1638 Compare November 16, 2022 19:33

andyyang890 requested review from a team and rafiss November 16, 2022 19:44

rafiss requested changes Nov 16, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch from b3a1638 to 0cfe823 Compare November 17, 2022 19:57

andyyang890 commented Nov 17, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch from 0cfe823 to 80f7816 Compare November 17, 2022 20:04

andyyang890 commented Nov 17, 2022

View reviewed changes

ZhouXing19 requested changes Nov 17, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch 6 times, most recently from c9ac159 to a91d4b7 Compare November 23, 2022 19:13

andyyang890 commented Nov 23, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch 6 times, most recently from d693359 to 83d507a Compare November 27, 2022 21:07

andyyang890 force-pushed the migrate_role_members branch 2 times, most recently from 6d90b41 to 8629d41 Compare December 2, 2022 16:46

andreimatei reviewed Dec 2, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch from 8629d41 to cd25722 Compare December 2, 2022 18:07

andyyang890 commented Dec 2, 2022

View reviewed changes

andreimatei reviewed Dec 2, 2022

View reviewed changes

andyyang890 commented Dec 2, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch from cd25722 to 746e7d7 Compare December 2, 2022 20:17

andyyang890 commented Dec 2, 2022

View reviewed changes

andyyang890 commented Dec 5, 2022

View reviewed changes

andyyang890 force-pushed the migrate_role_members branch from 746e7d7 to 7adcd34 Compare December 8, 2022 23:04

craig bot merged commit e1e3c3d into cockroachdb:master Dec 9, 2022

andyyang890 deleted the migrate_role_members branch December 9, 2022 02:56

Conversation

andyyang890 commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 8, 2022

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

ZhouXing19 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhouXing19 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhouXing19 commented Nov 17, 2022

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 commented Dec 1, 2022

Uh oh!

craig bot commented Dec 2, 2022

Uh oh!

craig bot commented Dec 2, 2022

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

ZhouXing19 commented Dec 2, 2022

Uh oh!

andyyang890 commented Dec 2, 2022

Uh oh!

craig bot commented Dec 2, 2022

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andreimatei left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 commented Dec 2, 2022

Uh oh!

craig bot commented Dec 2, 2022

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 left a comment

Choose a reason for hiding this comment

Uh oh!

andyyang890 commented Dec 9, 2022

Uh oh!

craig bot commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andyyang890 commented Nov 8, 2022 •

edited

Loading

ZhouXing19 left a comment •

edited

Loading

ZhouXing19 left a comment •

edited

Loading