sql: migration and mixed cluster version roachtest for grant option #72982

jackcwu · 2021-11-19T16:16:23Z

The roachtest ensures that the validation check for grant options that is performed on a grant/revoke only runs when all nodes on a cluster are upgraded to 22.1. The migration scans through existing users and sets their grant option bits equal to their privilege bits if they have GRANT or ALL privileges (otherwise, it would have been impossible for them to grant anyways, so we do not do anything).

cockroach-teamcity · 2021-11-19T16:16:37Z

This change is

RichardJCai

Adding my high level comments first

RichardJCai · 2021-11-30T21:17:43Z

pkg/sql/catalog/descpb/privilege.go

+	// Version22_1 corresponds to desciptors created in 22.1 and onwards.
+	// These descriptors should have grant options for all their privileges
+	// if they have the GRANT or ALL were created prior to 22.1.
+	Version22_1


I don't think we need to introduce two versions here

I think we only need GrantOptionVersion such that on GrantOptionVersion, we're guaranteed that the grant option bits are set (ie the migration has run or that the PrivilegeDesc was created on a version where grant option is supported)

RichardJCai · 2021-11-30T21:22:08Z

pkg/sql/catalog/descriptor.go

+	// above. The difference is that these changes will only every be run during the
+	// migration and will stop once all nodes are upgraded and waitForUpgradeStep(c.All())
+	// is called.
+	RunMigrationOnlyChanges(ctx context.Context, dg DescGetter) error


I'd rather call this MaybeAddWithGrantOptions and note that we only want it to be run during the migration you're adding.

RichardJCai · 2021-11-30T21:23:20Z

pkg/sql/catalog/catprivilege/fix.go

+func MaybeAddGrantOptions(ptr **descpb.PrivilegeDescriptor) bool {
+	p := *ptr
+	changed := false
+	if p.Version == descpb.GrantOptionVersion { // do not apply changes


This should be if p.Version >= descpb.GrantOptionVersion

RichardJCai · 2021-11-30T21:23:30Z

pkg/sql/catalog/catprivilege/fix.go

+	//	p.SetVersion(descpb.Version21_2)
+	//	changed = true
+	//}
+	p.SetVersion(descpb.Version22_1)


Set to GrantOptionVersion

RichardJCai · 2021-11-30T21:25:26Z

pkg/migration/migrations/grant_option_migration.go

+		return nil
+	}
+
+	return addGrantOptionMigration(ctx, rows, addGrantOptionFunc, 1<<19)


Add comment 1<<19 /* 512 KiB batch size */

jackcwu

Thank you, I made the fixes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @stevendanna)

pkg/migration/migrations/grant_option_migration.go, line 67 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Add comment 1<<19 /* 512 KiB batch size */

added

pkg/sql/catalog/descriptor.go, line 88 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I'd rather call this MaybeAddWithGrantOptions and note that we only want it to be run during the migration you're adding.

fixed

pkg/sql/catalog/catprivilege/fix.go, line 148 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

This should be if p.Version >= descpb.GrantOptionVersion

fixed

pkg/sql/catalog/catprivilege/fix.go, line 191 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Set to GrantOptionVersion

fixed

pkg/sql/catalog/descpb/privilege.go, line 54 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I don't think we need to introduce two versions here

I think we only need GrantOptionVersion such that on GrantOptionVersion, we're guaranteed that the grant option bits are set (ie the migration has run or that the PrivilegeDesc was created on a version where grant option is supported)

my bad, this was something I was testing out when I was previously trying to incorporate these changes into runPostDeserializationChanges and forgot to remove. Fixed

RichardJCai · 2021-12-01T21:05:06Z

pkg/cmd/roachtest/tests/privilege_version_upgrade.go

@@ -47,6 +47,11 @@ func runPrivilegeVersionUpgrade(
 	}

 	const currentVersion = ""
+	fmt.Println("<------------------------------------------------>")


Remove this please.

RichardJCai · 2021-12-01T21:05:54Z

pkg/cmd/roachtest/tests/validate_grant_option.go

+
+// execSQLUser changes the current user from "root" to the one that is input and executes the
+// SQL statement as that user.
+func execSQLUser(sqlStatement string, user string, errorExpected bool, node int) versionStep {


Rename to execAsUser and update comment to say it executes the statement as the specified user.

RichardJCai · 2021-12-01T21:07:34Z

pkg/cmd/roachtest/tests/validate_grant_option.go

+		t.Fatal(err)
+	}
+
+	u := newVersionUpgradeTest(c,


Can you add some comments here to make it easier to follow about what we're expecting at each "stage", ie what do we expect to happen before the migration runs, what do we expect to happen after the migration runs.

RichardJCai · 2021-12-01T21:07:47Z

pkg/migration/migrations/grant_option_migration.go

+)
+
+// grantOptionMigration iterates through every descriptor and sets a user's grant option bits
+// equal to its privilege bits if it holds the "GRANT" privilege


nit: period

RichardJCai · 2021-12-01T21:08:29Z

pkg/migration/migrations/grant_option_migration.go

+				return errors.Newf("unable to find descriptor for id %d", id)
+			}
+
+			err := b.MaybeAddGrantOptions(ctx, nil /* DescGetter */) // RunPostDeserializationChanges previously


Remove comment here

RichardJCai · 2021-12-01T21:10:25Z

pkg/sql/catalog/schemadesc/schema_desc_builder.go

+}
+
+// MaybeAddGrantOptions implements the catalog.DescriptorBuilder
+// interface


nit: period

RichardJCai · 2021-12-01T21:11:32Z

pkg/sql/catalog/tabledesc/table_desc_builder.go

+	var err error
+	tdb.maybeModified = protoutil.Clone(tdb.original).(*descpb.TableDescriptor)
+	tdb.changes, err = maybeFillInDescriptor(ctx, dg, tdb.maybeModified, tdb.skipFKsWithNoMatchingTable)
+	tdb.changes, err = maybeAddGrantOptionsToDescriptor(tdb.maybeModified)


Do we need this helper function instead of doing what the helper does directly?

RichardJCai · 2021-12-01T21:12:07Z

pkg/sql/catalog/catprivilege/fix.go

+
+// MaybeUpdateGrantOptions iterates over the users of the descriptor and checks
+// if they have the GRANT privilege - if so, then set the user's grant option
+// bits equal to the privilege bits


nit: period.

RichardJCai · 2021-12-01T21:12:36Z

pkg/sql/catalog/catprivilege/fix.go

+func MaybeUpdateGrantOptions(ptr **descpb.PrivilegeDescriptor) bool {
+	p := *ptr
+	changed := false
+	if p.Version >= descpb.GrantOptionVersion { // do not apply changes


nit: period and specify why we do not need to apply changes (on >= GrantOptionVersion means grant bits are set)

RichardJCai · 2021-12-01T21:14:18Z

cc @cockroachdb/sql-schema @ajwerner could y'all PTAL at the 2nd commit as well?

jackcwu

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @stevendanna)

pkg/cmd/roachtest/tests/privilege_version_upgrade.go, line 50 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove this please.

removed

pkg/cmd/roachtest/tests/validate_grant_option.go, line 50 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Rename to execAsUser and update comment to say it executes the statement as the specified user.

fixed

pkg/cmd/roachtest/tests/validate_grant_option.go, line 72 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Can you add some comments here to make it easier to follow about what we're expecting at each "stage", ie what do we expect to happen before the migration runs, what do we expect to happen after the migration runs.

added

pkg/migration/migrations/grant_option_migration.go, line 30 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: period

added

pkg/migration/migrations/grant_option_migration.go, line 50 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Remove comment here

removed

pkg/sql/catalog/catprivilege/fix.go, line 144 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: period.

added

pkg/sql/catalog/catprivilege/fix.go, line 148 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: period and specify why we do not need to apply changes (on >= GrantOptionVersion means grant bits are set)

added

pkg/sql/catalog/schemadesc/schema_desc_builder.go, line 71 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

nit: period

added

pkg/sql/catalog/tabledesc/table_desc_builder.go, line 119 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Do we need this helper function instead of doing what the helper does directly?

changed to call that in the function directly.

I realized that I accidentally left in tdb.changes, err = maybeFillInDescriptor(ctx, dg, tdb.maybeModified, tdb.skipFKsWithNoMatchingTable), which is something called in runPostDesserializationChanges. I believe this means that the error can always be nil for this function.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jackcwu, @RichardJCai, and @stevendanna)

-- commits, line 4 at r4:
nit: add newlines to the commit message. each line should be 80 characters

pkg/cmd/roachtest/tests/validate_grant_option.go, line 57 at r4 (raw file):

			require.NoError(t, err)
		} else {
			require.Error(t, err)

this line should also assert that the error is what we expect it to be. maybe instead of errorExpected the param could be expectedErrText (and if it's an empty string, then keep the require.NoError())

pkg/cmd/roachtest/tests/validate_grant_option.go, line 143 at r4 (raw file):

		execSQL("CREATE USER foo5;", false, 2),
		execSQL("CREATE TABLE t2();", false, 3),
		execSQL("GRANT ALL PRIVILEGES ON TABLE t2 TO foo5;", false, 1),

can there be one more check here to make sure that GRANT GRANT ... is turned into WITH GRANT OPTION after the upgrade?

and maybe also another one to make sure GRANT ... WITH GRANT OPTION works as expected

pkg/migration/migrations/grant_option_migration.go, line 50 at r3 (raw file):

Previously, jackcwu wrote…

removed

nit: remove commented code

pkg/migration/migrations/grant_option_migration.go, line 36 at r4 (raw file):

	query := `SELECT id, descriptor, crdb_internal_mvcc_timestamp FROM system.descriptor ORDER BY ID ASC`
	rows, err := d.InternalExecutor.QueryIterator(
		ctx, "update-grant-options", nil /* txn */, query,

nit: rename update-grant-options to retrieve-grant-options

pkg/sql/catalog/descriptor.go, line 88 at r2 (raw file):

Previously, jackcwu wrote…

fixed

nit: still needs to be renamed to MaybeAddWithGrantOptions

pkg/sql/catalog/catprivilege/fix.go, line 162 at r4 (raw file):

	// give ALL to the grant option bits for the root and admin users since they are superusers.
	superuserAllGrantOptions(security.RootUserName())

we don't really need this step, right? since root/admin skips privilege checks?

jackcwu

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @RichardJCai, and @stevendanna)

-- commits, line 4 at r4:

Previously, rafiss (Rafi Shamim) wrote…

nit: add newlines to the commit message. each line should be 80 characters

fixed

pkg/cmd/roachtest/tests/validate_grant_option.go, line 57 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this line should also assert that the error is what we expect it to be. maybe instead of errorExpected the param could be expectedErrText (and if it's an empty string, then keep the require.NoError())

added

pkg/cmd/roachtest/tests/validate_grant_option.go, line 143 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can there be one more check here to make sure that GRANT GRANT ... is turned into WITH GRANT OPTION after the upgrade?

and maybe also another one to make sure GRANT ... WITH GRANT OPTION works as expected

added

pkg/migration/migrations/grant_option_migration.go, line 50 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: remove commented code

removed

pkg/migration/migrations/grant_option_migration.go, line 36 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: rename update-grant-options to retrieve-grant-options

fixed

pkg/sql/catalog/descriptor.go, line 88 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: still needs to be renamed to MaybeAddWithGrantOptions

changing this to runMigrationOnlyChanges instead as a generic method for that behavior

pkg/sql/catalog/catprivilege/fix.go, line 162 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we don't really need this step, right? since root/admin skips privilege checks?

true, removed

postamar

Sorry this slipped under my radar. This all looks good enough to me. Caveat: I don't know much about the behaviour expected of privileges.

If it's feasible, consider adding a test case in backupccl in which we check that restoring a backup from several versions ago produces correct privilege descriptors. I don't insist on it, but it would be a nice to-have, if it's easy to do.

Reviewed 1 of 7 files at r7, 1 of 5 files at r8, 3 of 6 files at r11, 9 of 13 files at r12, 7 of 7 files at r13, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jackcwu, @rafiss, and @RichardJCai)

pkg/migration/migrations/grant_option_migration.go, line 67 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i moved the logic into RunPostDeserializationChanges. I agree it should work, since grant options are additive and backward-compatible. I don't think I followed the second half of your suggestion, for how we can remove the old GRANT privilege. Let me know what else is needed.

FYI, I don't know enough about privileges to comment further in this thread.

pkg/sql/catalog/catprivilege/fix.go, line 145 at r13 (raw file):

// if they have the GRANT privilege - if so, then set the user's grant option
// bits equal to the privilege bits.
func MaybeUpdateGrantOptions(ptr **descpb.PrivilegeDescriptor) bool {

nit: This doesn't need to be a pointer to a pointer. We had to do this for MaybeFixPrivileges in case the privileges pointer was nil (in which case we make it point to a new empty struct) but when this function is called it's guaranteed to not be nil anymore.

pkg/migration/migrations/grant_option_migration_external_test.go, line 191 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

do you mean I should use tc.Server(0).CollectionFactory here?

Yes, although you might find it simpler to open a transaction with sql.DescsTxn which requires the ExecutorConfig. It's the same thing under the hood.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, @rafiss, and @RichardJCai)

pkg/sql/catalog/catprivilege/fix.go, line 145 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: This doesn't need to be a pointer to a pointer. We had to do this for MaybeFixPrivileges in case the privileges pointer was nil (in which case we make it point to a new empty struct) but when this function is called it's guaranteed to not be nil anymore.

ah ok, fixed

pkg/migration/migrations/grant_option_migration_external_test.go, line 191 at r9 (raw file):

Previously, postamar (Marius Posta) wrote…

Yes, although you might find it simpler to open a transaction with sql.DescsTxn which requires the ExecutorConfig. It's the same thing under the hood.

hm, actually, i need to keep using the catalogkv version here, since i need shouldRunPostDeserializationChanges=false

postamar

Reviewed 2 of 2 files at r14, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, and @RichardJCai)

pkg/migration/migrations/grant_option_migration_external_test.go, line 191 at r9 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

hm, actually, i need to keep using the catalogkv version here, since i need shouldRunPostDeserializationChanges=false

Oh, good point. Sorry for misleading you.

RichardJCai · 2022-01-24T16:15:32Z

pkg/migration/migrations/grant_option_migration.go

@@ -44,6 +44,23 @@ func grantOptionMigration(
 	addGrantOptionFunc := func(ids []descpb.ID, descs []descpb.Descriptor, timestamps []hlc.Timestamp) error {
 		var modifiedDescs []catalog.MutableDescriptor
 		for i, id := range ids {
+			// Temporarily change the version in order to force the migration to run


This is kind of suspect to me. Doesn't this make it so the migration can run twice - ie we don't have a base case for the descriptor state to reach?

Also are we sure the grant option bits are wiped out?

Doesn't this make it so the migration can run twice - ie we don't have a base case for the descriptor state to reach?

Sort of -- but the migration itself only runs when the clusterversion reaches clusterversion.ValidateGrantOption and that can only happen once.

Also are we sure the grant option bits are wiped out?

Yes, this was causing line 178 to fail here in validate_grant_option.go ( aafdb46#diff-edd6b676708815ac3b09d3f07bb9c8c4bffe7441dacc9452c6f7453c2ba194efR178 )

here's the reason it gets wiped out:

suppose you have a mixed-version cluster

run CREATE TABLE t on a node running the new binary. this creates a privilege descriptor with a version of GrantOptionVersion.

run GRANT GRANT, SELECT ON t TO testuser on a node running the new binary. testuser can now grant SELECT to others, and the descriptor has its grant options bits set.

run GRANT DELETE ON t TO testuser on a node running the old binary. now the descriptor has the GRANT, SELECT, and DELETE privileges, but the grant option bits are not set since the old binary cannot serialize a descriptor with that info.

finalize the cluster upgrade. without my change here, the grant option bits are not added, since the privilege descriptor already is at GrantOptionVersion. now testuser can NOT grant SELECT to others, even though they could right before the upgrade was finalized.

Ah I see, it's not wiping out the grant bit but the migration isn't happening as expected.

This migration is kind of thorny, I'm still a bit iffy on downgrading the version temporarily, it just seems scary since we generally always expect the version upgrade to increase.

What if in this step

run GRANT GRANT, SELECT ON t TO testuser on a node running the new binary. testuser can now grant SELECT to others, and the descriptor has its grant options bits set.

We just also lazily upgrade the descriptor to have GRANT bits, and continue keeping GRANT privilege as well as we still need it until we finalize the migration, then in the next version we'll have a migration if necessary to then fully remove GRANT option (we can't do it til then)

it just seems scary since we generally always expect the version upgrade to increase.

well, the version is only modified in memory here; we never write that back to disk.

We just also lazily upgrade the descriptor to have GRANT bits, and continue keeping GRANT privilege as well as we still need it until we finalize the migration

This is how it works right now.

it's not wiping out the grant bit but the migration isn't happening as expected.

It is wiping out the grant option bits

I view the temporary, in-memory change to the version as safe. It's just a way to avoid having to do another migration in the 22.1->22.2 upgrade. with the migration like this, in during the 22.2 we should be able to just stop referencing the GRANT privilege in the code without needing to do anything else. (we'd still have to keep the GRANT enum value, so that it never accidentally gets reused in the future.)

Sorry, I misunderstood. I see now, we moved some of the logic back into RunPostDeserializedChanges.

I'm still somewhat confused on how the grant bits get wiped, I thought since on the old node we wouldn't be updating the grant bits in the proto, they'd stay the same. Gonna check out the branch to double check my understanding.

My concern with downgrading the version is that it's kind of strange that we're doing the migration on privilege descriptors that shouldn't need the migration.

Ideally for me, new descriptors should not need the migration at all if they're created on that version.
Can we version gate the descriptor version as well? Only set the descriptor version to GrantOptionVersion iff the cluster verison is ValidateGrantOption?

I'm still somewhat confused on how the grant bits get wiped, I thought since on the old node we wouldn't be updating the grant bits in the proto, they'd stay the same.

it's because the old node's proto definition doesn't even have a field for the grant option bits. so when it reads the bytes and turns it into a privilege descriptor struct, it doesn't have that field set at all. and when it turns it back into bytes, it wouldn't even know how to serialize that field anyway.

new descriptors should not need the migration at all if they're created on that version.
Can we version gate the descriptor version as well? Only set the descriptor version to GrantOptionVersion iff the cluster verison is ValidateGrantOption?

yes this would work too. i didn't do it though since it would require a lot of plumbing, which we'd then want to undo in 22.2.

My concern with downgrading the version is that it's kind of strange that we're doing the migration on privilege descriptors that shouldn't need the migration.

Well the thing is, they do need the migration. If we go with your suggestion of setting GrantOptionVersion iff the cluster verison is ValidateGrantOption, then the migration would work the exact same way, in practice -- it would run on all privilege descriptors.

if it helps, i can do one more check for this "temporary version change" bit i added -- it only needs to run IFF there are no grant option bits currently set.

Yeah looking into it, the plumbing is pretty tedious. Let me think about it for a little more to see if we can avoid it, otherwise it's fine.

RichardJCai · 2022-01-25T15:52:39Z

pkg/sql/catalog/catprivilege/fix.go

+// bits equal to the privilege bits.
+func MaybeUpdateGrantOptions(p *descpb.PrivilegeDescriptor) bool {
+	changed := false
+	if p.Version >= descpb.GrantOptionVersion {


I think if we're decreasing the version, then we don't need this new version at all since this is the only place where the version is used and checked. I had envisioned adding this new version would make us run the migration only on the old descriptors but if we're decreasing the version regardless then it seems like this new version wouldn't be needed.

Perhaps it makes sense to do this check

if p != nil { // If root doesn't have these grant option bits set, then we know // that an old node must have clobbered the grant option bits. if !p.CheckGrantOptions(security.RootUserName(), privilege.List{privilege.ALL}) { p.Version = descpb.Version21_2 } }

here instead, if root has grant option bits then we can skip the migration?

hm yeah, that seems reasonable. the only point i could see is that setting the version makes things very explicit. but i think ultimately since this migration is not backwards incompatible, you're right we don't need a new version at all.

actually, i still think the GrantOptionVersion could be valuable for one more reason: for system tables, admin/root do not have ALL privileges or grant options. We don't want to run MaybeUpdateGrantOptions each time we read a system table descriptor, so incrementing the version allows us to avoid that.

We could directly use IsSystemID and skip if it is right?

no, that doesn't work since the admins should still get the SELECT grant option for the system tables; but i think we should be able to use a different check. admins should have the SELECT grant option on every table no matter what, so that should work

The roachtest ensures that the validation check for grant options that is performed on a grant/revoke only runs when all nodes on a cluster are upgraded to 22.1. The migration scans through existing users and sets their grant option bits equal to their privilege bits if they have GRANT or ALL privileges (otherwise, it would have been impossible for them to grant anyways, so we do not do anything). Release note: None

RichardJCai

Thanks for taking this over and doing the revisions, much appreciated. LGTM now!

rafiss · 2022-01-26T01:46:07Z

tftrs all!

bors r=postamar,RichardJCai

This changes the migration so that it runs on all the privilege descriptors that don't have grant options set. Additionally, the GRANT/REVOKE logic is changed so that it also applies the grant option if the grantee currently has the GRANT privilege. Release note: None

craig · 2022-01-26T04:11:28Z

Canceled.

rafiss · 2022-01-26T04:14:14Z

bors r=postamar,RichardJCai

craig · 2022-01-26T04:24:36Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-01-26T05:15:01Z

Build succeeded:

GitHub CI (Cockroach)

jackcwu requested review from rafiss, RichardJCai and a team November 19, 2021 16:16

jackcwu marked this pull request as ready for review November 29, 2021 20:47

jackcwu requested review from a team November 29, 2021 20:47

jackcwu requested a review from a team as a code owner November 29, 2021 20:47

jackcwu requested review from stevendanna and removed request for a team November 29, 2021 20:47

jackcwu marked this pull request as draft November 29, 2021 20:48

jackcwu force-pushed the grant_option_migration_roachtest_final branch from 1943aa7 to e352827 Compare November 30, 2021 20:42

jackcwu closed this Nov 30, 2021

jackcwu reopened this Nov 30, 2021

jackcwu marked this pull request as ready for review November 30, 2021 21:03

RichardJCai reviewed Nov 30, 2021

View reviewed changes

jackcwu force-pushed the grant_option_migration_roachtest_final branch from e352827 to c8df8d5 Compare December 1, 2021 16:43

jackcwu commented Dec 1, 2021

View reviewed changes

jackcwu requested a review from RichardJCai December 1, 2021 16:44

RichardJCai reviewed Dec 1, 2021

View reviewed changes

jackcwu force-pushed the grant_option_migration_roachtest_final branch from c8df8d5 to 4d461b4 Compare December 2, 2021 16:41

jackcwu commented Dec 2, 2021

View reviewed changes

rafiss mentioned this pull request Dec 6, 2021

sql: support WITH GRANT OPTION when granting privileges #67410

Closed

jackcwu force-pushed the grant_option_migration_roachtest_final branch 2 times, most recently from 0786b1a to 2136cc0 Compare December 7, 2021 19:51

rafiss requested changes Dec 9, 2021

View reviewed changes

jackcwu force-pushed the grant_option_migration_roachtest_final branch from 2136cc0 to 1943aa7 Compare December 13, 2021 18:19

jackcwu commented Dec 13, 2021

View reviewed changes

jackcwu force-pushed the grant_option_migration_roachtest_final branch from 1943aa7 to 7fa6a74 Compare December 13, 2021 18:21

rafiss removed the request for review from a team January 19, 2022 16:46

postamar approved these changes Jan 20, 2022

View reviewed changes

rafiss force-pushed the grant_option_migration_roachtest_final branch from d224830 to 6c05e1c Compare January 20, 2022 16:32

rafiss reviewed Jan 21, 2022

View reviewed changes

postamar reviewed Jan 21, 2022

View reviewed changes

rafiss force-pushed the grant_option_migration_roachtest_final branch 2 times, most recently from 49a2287 to 1ccfd65 Compare January 24, 2022 05:05

RichardJCai reviewed Jan 24, 2022

View reviewed changes

rafiss force-pushed the grant_option_migration_roachtest_final branch from 1ccfd65 to aafdb46 Compare January 24, 2022 16:43

rafiss requested a review from RichardJCai January 24, 2022 18:21

rafiss force-pushed the grant_option_migration_roachtest_final branch from aafdb46 to 4a16f13 Compare January 24, 2022 22:56

RichardJCai reviewed Jan 25, 2022

View reviewed changes

rafiss force-pushed the grant_option_migration_roachtest_final branch 3 times, most recently from 47d7402 to 087f70a Compare January 25, 2022 21:44

RichardJCai self-requested a review January 25, 2022 21:51

RichardJCai approved these changes Jan 25, 2022

View reviewed changes

rafiss force-pushed the grant_option_migration_roachtest_final branch 2 times, most recently from 598fbb7 to a6dc666 Compare January 26, 2022 01:45

rafiss force-pushed the grant_option_migration_roachtest_final branch from a6dc666 to 5b77b33 Compare January 26, 2022 04:11

craig bot merged commit e5d1c37 into cockroachdb:master Jan 26, 2022

sql: migration and mixed cluster version roachtest for grant option #72982

sql: migration and mixed cluster version roachtest for grant option #72982

Conversation

jackcwu commented Nov 19, 2021 • edited by rafiss Loading

cockroach-teamcity commented Nov 19, 2021

RichardJCai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcwu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardJCai commented Dec 1, 2021

jackcwu left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jackcwu left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafiss Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

rafiss commented Jan 26, 2022

craig bot commented Jan 26, 2022

rafiss commented Jan 26, 2022

craig bot commented Jan 26, 2022

craig bot commented Jan 26, 2022

jackcwu commented Nov 19, 2021 •

edited by rafiss

Loading

rafiss Jan 24, 2022 •

edited

Loading