sql/schemachanger: implement DROP OWNED BY #82936

jasonmchan · 2022-06-15T14:43:10Z

Previously, we did not support the DROP OWNED BY statement (#55381).
This commit adds partial support for DROP OWNED BY in the declarative
schema changer. Followup work is needed to support the CASCADE modifier.

Release note (sql change): Support DROP OWNED BY.

cockroach-teamcity · 2022-06-15T14:43:20Z

This change is

Xiang-Gu

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasonmchan and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 44 at r1 (raw file):

	var toCheckBackrefs []descpb.ID

	// Lookup all objects in the current database.

curious: can a user own objects across databases? If so, what's the expected behavior if we DRO OWNED BY him?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 56 at r1 (raw file):

		})
		objects = append(objects, sp.SchemaID)
	})

Is this block used to retrieve all object IDs in db?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 61 at r1 (raw file):

	for _, id := range objects {
		elts := b.QueryByID(id)
		_, _, owner := scpb.FindOwner(elts)

is it possible that elts does not have a Owner element?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 88 at r1 (raw file):

		for _, role := range normalizedRoles {
			if e.UserName == role.Normalized() && e.UserName != b.SessionData().User().Normalized() {
				dropElement(b, e)

you probably need to change this to simply b.Drop(e) with #82907 coming out soon.

pkg/sql/logictest/testdata/logic_test/drop_owned_by line 83 at r1 (raw file):

SHOW TABLES FROM public
----

when testuser drops all objects he owned, why are u and v1 also dropped (which are owned by testuser2）?
in addition to dropping the objects, we also revoke the privileges from users. Do we tests this behavior in the logic test?

pkg/sql/logictest/testdata/logic_test/drop_owned_by line 212 at r1 (raw file):


statement error pq: cannot drop objects owned by role "admin" because they are required by the database system
DROP OWNED BY admin

Can we add some more comments on the test? It's a bit hard to follow and remember what's been dropped and what's been created by which user.

You can break up the tests into subtests and each subtest focus on a pattern/theme/case of tests.

Xiang-Gu

Good job! Keep up the good work!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jasonmchan and @Xiang-Gu)

jasonmchan

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 44 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

curious: can a user own objects across databases? If so, what's the expected behavior if we DRO OWNED BY him?

Yes. DROP OWNED BY drops objects in the current database (see Postgres docs)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 56 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

Is this block used to retrieve all object IDs in db?

Yes, would it be clearer if I renamed objects to objectIDs?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 61 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

is it possible that elts does not have a Owner element?

I've been working under this assumption, but maybe someone else can confirm. My understanding is that all objects have owners, which is enforced when we decompose descriptors into elements.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 88 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

you probably need to change this to simply b.Drop(e) with #82907 coming out soon.

Thanks for letting me know! I didn't pay close attention to the difference 😅

pkg/sql/logictest/testdata/logic_test/drop_owned_by line 83 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

when testuser drops all objects he owned, why are u and v1 also dropped (which are owned by testuser2）?

in addition to dropping the objects, we also revoke the privileges from users. Do we tests this behavior in the logic test?

The previous statement DROP OWNED BY testuser2 drops them. The intention here was to test CASCADE; I've added a TODO comment to indicate this.
Yes

pkg/sql/logictest/testdata/logic_test/drop_owned_by line 212 at r1 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

Can we add some more comments on the test? It's a bit hard to follow and remember what's been dropped and what's been created by which user.

You can break up the tests into subtests and each subtest focus on a pattern/theme/case of tests.

I've attempted to improve the readability of the tests. Hopefully they're clearer now!

chengxiong-ruan

Nice work :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @jasonmchan, and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/dependencies.go line 172 at r2 (raw file):

	// MemberOfWithAdminOption looks up the roles that 'member' is a member of.
	MemberOfWithAdminOption(ctx context.Context, member username.SQLUsername) (map[username.SQLUsername]bool, error)

the go convention is to use map[username.SQLUsername]struct{} here to avoid the confusion of true and false boolean values :)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 177 at r2 (raw file):

	// CheckMemberOf returns true iff the current user has membership in the
	// specified role.
	IsMemberOf(member username.SQLUsername) bool

IsMemberOf -> CurrentUserIsMemberOf?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 88 at r1 (raw file):

Previously, jasonmchan (Jason Chan) wrote…

Thanks for letting me know! I didn't pay close attention to the difference 😅

ha, nice

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 27 at r2 (raw file):

// DropOwnedBy implements DROP OWNED BY.
func DropOwnedBy(b BuildCtx, n *tree.DropOwnedBy) {
	normalizedRoles, err := decodeusername.FromRoleSpecList(

what does normalized mean here?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 36 at r2 (raw file):

		if role.IsAdminRole() || role.IsRootUser() || role.IsNodeUser() {
			panic(pgerror.Newf(pgcode.DependentObjectsStillExist,
				"cannot drop objects owned by role %q because they are required by the database system", role))

Is it legit to drop non-system objects though?

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 39 at r2 (raw file):

		}
		if role != b.SessionData().User() && !b.IsMemberOf(role) {
			panic(pgerror.New(pgcode.InsufficientPrivilege, "permission denied to drop objects"))

Is it legit for admin/root perform this operation if they are not part the input roles?

pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags line 249 at r2 (raw file):

REASSIGN OWNED BY root TO testuser

# Test DROP OWNED BY.

Could you add DROP OWNED BY a special case here as well so that it still use its SchemaFeatureName which is DROP OWNED BY?:

cockroach/pkg/sql/sem/tree/schema_feature_name.go

Line 26 in 4ee236c

case *CommentOnDatabase, *CommentOnSchema, *CommentOnTable,

?
We'd need to refactor this better somehow. I haven't thought about it yet :(

jasonmchan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/dependencies.go line 172 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

the go convention is to use map[username.SQLUsername]struct{} here to avoid the confusion of true and false boolean values :)

My comment was misleading, the boolean value actually indicates whether the member is an admin of the role. I've updated the comment. Leaving the signature as is to maintain parity with the AuthorizationAccessor in authorization.go.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go line 177 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

IsMemberOf -> CurrentUserIsMemberOf?

Done.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 27 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

what does normalized mean here?

It's referring to the username being normalized.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 36 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Is it legit to drop non-system objects though?

Postgres won't drop non-system objects for these roles. I'm not sure if there's a case where these roles don't own any system objects and so the statement would succeed, but I think that erroring here is the safe option.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 39 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Is it legit for admin/root perform this operation if they are not part the input roles?

Yes. The check for this is in the CurrentUserIsMemberOf function, although maybe it would be clearer if the check were elsewhere. Here's a logictest for this scenario.

pkg/sql/logictest/testdata/logic_test/schema_change_feature_flags line 249 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Could you add DROP OWNED BY a special case here as well so that it still use its SchemaFeatureName which is DROP OWNED BY?:

cockroach/pkg/sql/sem/tree/schema_feature_name.go

Line 26 in 4ee236c

case *CommentOnDatabase, *CommentOnSchema, *CommentOnTable,

?
We'd need to refactor this better somehow. I haven't thought about it yet :(

Done.

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @jasonmchan, and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/dependencies.go line 172 at r2 (raw file):

Previously, jasonmchan (Jason Chan) wrote…

My comment was misleading, the boolean value actually indicates whether the member is an admin of the role. I've updated the comment. Leaving the signature as is to maintain parity with the AuthorizationAccessor in authorization.go.

ah, interesting. thanks for explaining that.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 36 at r2 (raw file):

Previously, jasonmchan (Jason Chan) wrote…

Postgres won't drop non-system objects for these roles. I'm not sure if there's a case where these roles don't own any system objects and so the statement would succeed, but I think that erroring here is the safe option.

yeah, that make sense, thanks.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 39 at r2 (raw file):

Previously, jasonmchan (Jason Chan) wrote…

Yes. The check for this is in the CurrentUserIsMemberOf function, although maybe it would be clearer if the check were elsewhere. Here's a logictest for this scenario.

Hmm. wait.. I thought CurrentUserIsMemberOf only checks if current is a member of role, but don't really care about if it's an admin or not?
Oh.. does MemberOfWithAdminOption actually also return admin even the role is not an admin? heh...

pkg/sql/schemachanger/scplan/testdata/drop_owned_by line 1 at r4 (raw file):

setup

Sorry, I should have noticed this earlier but just realized that we need a builder test similar to these as well

jasonmchan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @jasonmchan, and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 39 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Hmm. wait.. I thought CurrentUserIsMemberOf only checks if current is a member of role, but don't really care about if it's an admin or not?
Oh.. does MemberOfWithAdminOption actually also return admin even the role is not an admin? heh...

It's actually because I put a separate check here for the admin case. I renamed the function to CurrentUserHasAdminOrIsMemberOf, is this better?

pkg/sql/schemachanger/scplan/testdata/drop_owned_by line 1 at r4 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

Sorry, I should have noticed this earlier but just realized that we need a builder test similar to these as well

Done.

chengxiong-ruan

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chengxiong-ruan, @jasonmchan, and @Xiang-Gu)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_owned_by.go line 39 at r2 (raw file):

Previously, jasonmchan (Jason Chan) wrote…

It's actually because I put a separate check here for the admin case. I renamed the function to CurrentUserHasAdminOrIsMemberOf, is this better?

Thanks that's much better!

jasonmchan · 2022-06-23T18:44:26Z

bors r+

postamar

This looks good, nicely done! I have a few comments but nothing big, they should be straightforward for you to address

postamar · 2022-06-23T18:48:38Z

pkg/sql/logictest/testdata/logic_test/drop_owned_by

+DROP OWNED BY root
+
+statement error pq: cannot drop objects owned by role "admin" because they are required by the database system
+DROP OWNED BY admin


I would like to see more test cases involving:

databases

schemas

tables, views and types referencing each other but owned by different roles.

The latter case is arguably more interesting for CASCADE but still.

Since you're not implementing CASCADE you should probably have a case that checks for a nice error message (search for statement error unimplemented: for examples). Also add a case in the unimplemented_drop data-driven test suite in the scbuild package.

I changed the unimplemented error from the one in scerror to the one in unimplemented, since it seems like the former causes us to try to fall back to the old schema changer, which ends up throwing a less descriptive error message for DROP OWNED BY. I added the logictest check but not the unimplemented_drop one--is this the right call?

Yeah, I hear you. I'm good with this approach since we're going to implement it anyway. But I'm wondering if you could just set new schema changer's mode to unsafe_always since you disable the legacy schema changer configuration in this logic test :) also cc @postamar if you have strong opinion on the options here.

I like your idea @chengxiong-ruan. The purpose of these tests is to check which schema changes aren't implemented by the declarative schema changer. It would be fine to ignore the legacy schema changer, in my opinion.

I don't have strong opinions either. @jasonmchan please do what you think is best and go ahead and bors.

postamar · 2022-06-23T19:08:13Z

pkg/sql/schemachanger/scop/mutation.go

+	mutationOp
+	DescID descpb.ID
+	User   string
+}


User privileges are implicitly removed when dropping something, so you can remove this op altogether.

I believe DROP OWNED BY should revoke user privileges for the role(s) on objects that aren't owned, so we need this op to drop those privileges without dropping the entire objects.

Oh, right. In that case, you should update the rules to avoid emitting this op for dropped objects. Look for op_rules.go.

Good catch. @jasonmchan see rules in pkg/sql/schemachanger/scplan/internal/rules/op_drop.go

I think this rule should handle this, since user privileges are part of the descriptor.

Amazing. Good find.

postamar · 2022-06-23T19:53:59Z

bors r-

Sorry for the late review! Please address my comments and feel free to re-bors.

craig · 2022-06-23T19:54:01Z

Canceled.

postamar · 2022-06-28T10:53:52Z

@jasonmchan feel free to go ahead and bors this again.

jasonmchan · 2022-06-28T14:50:30Z

bors r+

craig · 2022-06-28T15:49:37Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-06-28T16:23:09Z

Build failed (retrying...):

GitHub CI (Cockroach)

ajwerner · 2022-06-28T16:24:42Z

I think this has some merge skew on generated output.

bors r-

craig · 2022-06-28T16:24:44Z

Canceled.

Previously, we did not support the DROP OWNED BY statement (cockroachdb#55381). This commit adds partial support for DROP OWNED BY in the declarative schema changer. Followup work is needed to support the CASCADE modifier. Release note (sql change): Support `DROP OWNED BY`.

jasonmchan · 2022-06-28T17:53:28Z

Merge conflict should be fixed!

bors r+

craig · 2022-06-28T20:33:34Z

Build succeeded:

GitHub CI (Cockroach)

RichardJCai · 2022-07-08T04:09:19Z

Can we close #55381 with this?

We have a separate issue for support CASCADE.
#55908

postamar · 2022-07-08T13:00:58Z

By all means!

Xiang-Gu suggested changes Jun 15, 2022

View reviewed changes

Xiang-Gu reviewed Jun 15, 2022

View reviewed changes

jasonmchan force-pushed the drop-owned-by branch from eb377e6 to 37304b7 Compare June 16, 2022 17:32

jasonmchan commented Jun 16, 2022

View reviewed changes

jasonmchan marked this pull request as ready for review June 16, 2022 17:42

jasonmchan requested a review from a team June 16, 2022 17:42

jasonmchan force-pushed the drop-owned-by branch from 37304b7 to f93df50 Compare June 21, 2022 14:50

chengxiong-ruan reviewed Jun 21, 2022

View reviewed changes

jasonmchan commented Jun 21, 2022

View reviewed changes

jasonmchan force-pushed the drop-owned-by branch from f93df50 to 44f2326 Compare June 21, 2022 22:05

jasonmchan requested a review from a team as a code owner June 21, 2022 22:05

chengxiong-ruan reviewed Jun 22, 2022

View reviewed changes

jasonmchan force-pushed the drop-owned-by branch from 44f2326 to 3dac516 Compare June 23, 2022 16:06

jasonmchan commented Jun 23, 2022

View reviewed changes

chengxiong-ruan approved these changes Jun 23, 2022

View reviewed changes

postamar approved these changes Jun 23, 2022

View reviewed changes

Xiang-Gu approved these changes Jun 23, 2022

View reviewed changes

jasonmchan force-pushed the drop-owned-by branch from 3dac516 to c9edde5 Compare June 24, 2022 18:53

jasonmchan force-pushed the drop-owned-by branch from c9edde5 to 421856b Compare June 28, 2022 16:39

craig bot merged commit 5541cf8 into cockroachdb:master Jun 28, 2022

cockroach-teamcity mentioned this pull request Jun 28, 2022

sql/schemachanger: implement DROP OWNED BY cockroachdb/docs#14388

Closed

postamar mentioned this pull request Jul 8, 2022

sql: support DROP OWNED BY command #55381

Closed

sql/schemachanger: implement DROP OWNED BY #82936

sql/schemachanger: implement DROP OWNED BY #82936

Conversation

jasonmchan commented Jun 15, 2022 • edited Loading

cockroach-teamcity commented Jun 15, 2022

Xiang-Gu left a comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

jasonmchan left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

jasonmchan left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

jasonmchan left a comment

Choose a reason for hiding this comment

chengxiong-ruan left a comment

Choose a reason for hiding this comment

jasonmchan commented Jun 23, 2022

postamar left a comment • 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

postamar Jun 28, 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

postamar commented Jun 23, 2022

craig bot commented Jun 23, 2022

postamar commented Jun 28, 2022

jasonmchan commented Jun 28, 2022

craig bot commented Jun 28, 2022

craig bot commented Jun 28, 2022

ajwerner commented Jun 28, 2022

craig bot commented Jun 28, 2022

jasonmchan commented Jun 28, 2022

craig bot commented Jun 28, 2022

RichardJCai commented Jul 8, 2022

postamar commented Jul 8, 2022

jasonmchan commented Jun 15, 2022 •

edited

Loading

postamar left a comment •

edited

Loading

postamar Jun 28, 2022 •

edited

Loading