-
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: implement CREATE SCHEMA ... AUTHORIZATION
#53583
Conversation
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.
Reviewed 6 of 6 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @lucy-zhang, @rohany, and @solongordon)
pkg/sql/create_schema.go, line 73 at r1 (raw file):
Quoted 4 lines of code…
// Check validity of the schema name. if err := schemadesc.IsSchemaNameValid(schemaName); err != nil { return err }
Should we do this check before the existence check?
pkg/sql/parser/sql.y, line 5297 at r1 (raw file):
// %Category: DDL // %Text: // CREATE SCHEMA [IF NOT EXISTS] <schemaname> [AUTHORIZATION <rolename>]
nit: should this be CREATE SCHEMA [IF NOT EXISTS] { <schemaname> | [ [<schemaname>] AUTHORIZATION <rolename>] }
Done, addressed both comments bors r+ |
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 (and 1 stale) (waiting on @ajwerner, @lucy-zhang, @rohany, and @solongordon)
pkg/sql/create_schema.go, line 59 at r2 (raw file):
// Check validity of the schema name. if err := schemadesc.IsSchemaNameValid(schemaName); err != nil {
seems like re-arranging this broke some tests 😬
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 (and 1 stale) (waiting on @ajwerner, @lucy-zhang, @rohany, and @solongordon)
pkg/sql/parser/sql.y, line 5297 at r2 (raw file):
// %Category: DDL // %Text: // CREATE SCHEMA [IF NOT EXISTS] { <schemaname> | [<schemaname>] [AUTHORIZATION <rolename>] }
also, I don't think this means the same thing as what I wrote. This seems to imply that you could omit both elements of the RHS. I think you could do:
CREATE SCHEMA [IF NOT EXISTS] { <schemaname> | [<schemaname>] AUTHORIZATION <rolename> }
Ah yeah you're right, can't have brackets around the |
this is failing ci - removing from the bors queue bors r- |
Canceled. |
Fixes cockroachdb#53559. This commit adds the `CREATE SCHEMA ... AUTHORIZATION` command. When authorization is provided, the target user is given ownership of the schema. If the schema name is not provided, then the schema is named the same name as the target role. Release justification: low risk updates to new functionality Release note (sql change): Support the `CREATE SCHEMA ... AUTHORIZATION` command.
Updated the help message. Seems like we need to keep the name check after the existence check. |
bors r+ |
bors r- |
Canceled. |
@ajwerner can you stamp this? |
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.
Reviewed 4 of 6 files at r1, 2 of 2 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rohany and @solongordon)
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! 1 of 0 LGTMs obtained (waiting on @rohany and @solongordon)
bors r+ |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
Fixes #53559.
This commit adds the
CREATE SCHEMA ... AUTHORIZATION
command. Whenauthorization is provided, the target user is given ownership of the
schema. If the schema name is not provided, then the schema is named the
same name as the target role.
Release justification: low risk updates to new functionality
Release note (sql change): Support the
CREATE SCHEMA ... AUTHORIZATION
command.