Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql/schemachanger: Drop function in declarative schema changer #95153

Merged
merged 3 commits into from
Jan 27, 2023
Merged

sql/schemachanger: Drop function in declarative schema changer #95153

merged 3 commits into from
Jan 27, 2023

Conversation

chengxiong-ruan
Copy link
Contributor

@chengxiong-ruan chengxiong-ruan commented Jan 12, 2023 •
edited
Loading

This pr contains 3 commits:

  1. add function descriptor elements definition
  2. a simple rename of RemoveViewBackReferencesInRelation to RemoveBackReferencesInRelation
  3. drop function logics
    Part of: https://cockroachlabs.atlassian.net/browse/CRDB-19495

Fixes: #83235

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch 7 times, most recently from c2857a1 to fd28925 Compare January 13, 2023 02:13
@chengxiong-ruan chengxiong-ruan marked this pull request as ready for review January 13, 2023 14:47
@chengxiong-ruan chengxiong-ruan requested a review from a team as a code owner January 13, 2023 14:47
@chengxiong-ruan chengxiong-ruan requested review from a team and rhu713 and removed request for a team and rhu713 January 13, 2023 14:47
@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch 3 times, most recently from 4addadd to f08485e Compare January 13, 2023 20:13
@chengxiong-ruan chengxiong-ruan changed the title Drop function declarative sql/schemachanger: Drop function in declarative schema changer Jan 17, 2023
@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch 2 times, most recently from be3b07d to b6d46c5 Compare January 19, 2023 21:31
Xiang-Gu
Xiang-Gu reviewed Jan 19, 2023
View reviewed changes
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/drop_function.go
}
for _, f := range n.Functions {
elts := b.ResolveUDF(&f, ResolveParams{
IsExistenceOptional: n.IfExists,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't here be another requiredPrivilege : DROP for b.ResolveUDF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function only has EXECUTE privilege. Only function owner can drop the function or perform any schema changes on function. So we have b.mustOwn(fnID) in ResolveUDF to validate that

Xiang-Gu
Xiang-Gu reviewed Jan 19, 2023
View reviewed changes
pkg/sql/schemachanger/sctest_generated_test.go
Comment on lines +468 to +472
func TestRollback_drop_function(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
sctest.Rollback(t, "pkg/sql/schemachanger/testdata/end_to_end/drop_function", sctest.SingleNodeCluster)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have rollback test here but I didn't find any output text files in explain/explain_verbose directory. Is this right?

Xiang-Gu
Xiang-Gu reviewed Jan 19, 2023
View reviewed changes
pkg/sql/schemachanger/scplan/testdata/drop_view Outdated
@@ -15,7 +15,11 @@ StatementPhase stage 1 of 1 with 1 MutationType op
ops:
*scop.MarkDescriptorAsSyntheticallyDropped
DescriptorID: 105
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix all the merge conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. I messed it up. done.

Xiang-Gu
Xiang-Gu reviewed Jan 19, 2023
View reviewed changes
pkg/sql/schemachanger/scplan/internal/rules/op_drop.go Outdated
Comment on lines 243 to 246
(*scpb.FunctionName)(nil),
(*scpb.FunctionVolatility)(nil),
(*scpb.FunctionLeakProof)(nil),
(*scpb.FunctionNullInputBehavior)(nil),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about *scop.FunctionBody?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't skip the op for FunctionBody because we need to handle cross references removals.

Xiang-Gu
Xiang-Gu reviewed Jan 19, 2023
View reviewed changes
pkg/sql/schemachanger/scplan/internal/opgen/opgen_object_parent.go
Comment on lines +33 to +38
emit(func(this *scpb.ObjectParent) *scop.RemoveObjectParent {
return &scop.RemoveObjectParent{
ObjectID: this.ObjectID,
ParentSchemaID: this.ParentSchemaID,
}
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to change this part at all, no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm

@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch 3 times, most recently from d0ebaa0 to 6d5135f Compare January 19, 2023 23:15
@postamar
Copy link
Contributor

Sorry for the slow turnaround on my end. I'll review this on Monday!

@postamar
Copy link
Contributor

If it's not too much trouble I'd prefer it if this PR waited until #95631 was done. I'm running out of time before going away on leave and the rebase is likely to hurt.

@chengxiong-ruan
Copy link
Contributor Author

If it's not too much trouble I'd prefer it if this PR waited until #95631 was done. I'm running out of time before going away on leave and the rebase is likely to hurt.

Totally understood.

@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch 3 times, most recently from 9718ffe to 08bad65 Compare January 27, 2023 03:08
@chengxiong-ruan
sql/schemachanger: decomp function elements
9eee6ac 
This commit adds function element definitions and decomposition
logics.

Release note: None
@chengxiong-ruan
sql/schemachanger: rename RemoveViewBackReferencesInRelations
468b907 
This should work for both view and function, so renaming it
to RemoveBackReferencesInRelations.

Release note: None
@chengxiong-ruan
sql/schemachanger: declarative drop function
07ba6fb 
This commit implements DROP FUNCTION given existing
declarative schema changer infra (rules and ops).

`RemoveObjectParent` is added to handle the removal
of function signature from schema.

Release note: None
@chengxiong-ruan chengxiong-ruan force-pushed the drop-function-declarative branch from 08bad65 to 07ba6fb Compare January 27, 2023 15:25
@chengxiong-ruan
Copy link
Contributor Author

TFTR!
bors r+

@craig craig bot merged commit b23943c into cockroachdb:master Jan 27, 2023
@craig
Copy link
Contributor

craig bot commented Jan 27, 2023

Build succeeded:

This was referenced Jan 27, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xiang-Gu Xiang-Gu Xiang-Gu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

schemachanger: Support DROP FUNCTION statement in declarative schema changer
4 participants
@chengxiong-ruan @cockroach-teamcity @postamar @Xiang-Gu