User-defined schemas #8273
User-defined schemas #8273
Conversation
|
This change is |
ericharmeling
force-pushed
the
user-defined-schemas
branch
from
50a6d50 to
8687076
Compare
|
Friendly review reminder :) |
|
I'll get to this today. |
|
It might also be good to get someone on SQL Features to look at this. Maybe @arulajmani or @solongordon? |
|
I can take a look later today. |
|
Awesome. Thanks, y'all! |
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @ericharmeling)
v20.2/alter-schema.md, line 3 at r1 (raw file):
--- title: ALTER SCHEMA summary: The ALTER SCHEMA statement modifies a user-defined data type in a database.
s/data type/schema
v20.2/alter-schema.md, line 13 at r1 (raw file):
~~~ ALTER SCHEMA ... RENAME TO <newschemaname> ALTER SCHEMA ... OWNER TO {<newowner> | CURRENT_USER | SESSION_USER }
This isn't specific to schemas and maybe isn't even a documentation issue per se, but I'm confused about what the {<newowner> | CURRENT_USER | SESSION_USER } part means, since something like ALTER SCHEMA sc OWNER TO CURRENT_USER gives me an error about there not being a user current_user instead of recognizing this syntax. How is this syntax supposed to work? cc @arulajmani
v20.2/create-schema.md, line 3 at r1 (raw file):
--- title: CREATE SCHEMA summary: The CREATE SCHEMA statement creates a new, user-defined schema.
nit: I don't think this needs a comma.
v20.2/create-schema.md, line 7 at r1 (raw file):
--- <span class="version-tag">New in v20.2</span>: The `CREATE SCHEMA` [statement](sql-statements.html) creates a user-defined [schema](sql-name-resolution.html#logical-schemas-and-namespaces). With multiple schemas, you can create databases of the same name that belong to separate schemas.
I'm not sure what this last sentence means. Should that be "tables" instead of "databases"?
There was a problem hiding this comment.
Reviewable status:
v20.2/alter-schema.md, line 13 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
This isn't specific to schemas and maybe isn't even a documentation issue per se, but I'm confused about what the
{<newowner> | CURRENT_USER | SESSION_USER }part means, since something likeALTER SCHEMA sc OWNER TO CURRENT_USERgives me an error about there not being a usercurrent_userinstead of recognizing this syntax. How is this syntax supposed to work? cc @arulajmani
This definitely looks like a bug, yeah. Nice catch. I've filed cockroachdb/cockroach#54696 for investigation.
v20.2/create-schema.md, line 16 at r1 (raw file):
~~~ CREATE SCHEMA [IF NOT EXISTS] { <schemaname> | [<schemaname>] AUTHORIZATION {user_name | CURRENT_USER | SESSION_USER} }
Similar to @lucy-zhang's catch above, this CURRENT_USER/SESSION_USER stuff doesn't play well either because of cockroachdb/cockroach#54696, not sure if this will affect docs.
v20.2/create-schema.md, line 140 at r1 (raw file):
~~~ If no schema name is specified in a `CREATE SCHEMA` statement with an `AUTHORIZATION` clause, the schema will be named after the user specified:
Maybe it's worth extending this example by creating two tables with the same name, one in the public schema and one in the $user schema and showing how the name resolution works in that case? See my point/linked PR in the name resolution section for v20.2
v20.2/sql-name-resolution.md, line 46 at r1 (raw file):
1. If the name already fully specifies the database and schema, use that. 2. If the name has a single component prefix, try to find a schema with that name. If no such schema exists, use the `public` schema in the database with the prefix name.
I think recently we added a fix cockroachdb/cockroach#54538 which might impact this section as well.
ericharmeling
force-pushed
the
user-defined-schemas
branch
from
8687076 to
3010ad9
Compare
There was a problem hiding this comment.
TFTR @arulajmani @lucy-zhang
I can either merge this ahead of cockroachdb/cockroach#54696 being resolved (i.e., publish documentation of desired behavior, but not current behavior), or we can wait until that issue is resolved before merging this.
Reviewable status:
v20.2/alter-schema.md, line 3 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
s/data type/schema
Done.
v20.2/create-schema.md, line 3 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
nit: I don't think this needs a comma.
Done.
v20.2/create-schema.md, line 7 at r1 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I'm not sure what this last sentence means. Should that be "tables" instead of "databases"?
I removed this sentence, and made a new example to demonstrate that you can create two tables of the same name in separate schemas.
v20.2/create-schema.md, line 140 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Maybe it's worth extending this example by creating two tables with the same name, one in the public schema and one in the
$userschema and showing how the name resolution works in that case? See my point/linked PR in the name resolution section for v20.2
Done.
v20.2/sql-name-resolution.md, line 46 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I think recently we added a fix cockroachdb/cockroach#54538 which might impact this section as well.
Thanks for the heads-up!
Updated this page.
|
I'm leaning towards publishing this as-is unless @lucy-zhang has reservations. This stuff is mostly about user defined schemas and the ownership concept is orthogonal to this. Also, the ownership stuff is broken everywhere (not just schemas) -- you can't use CURRENT_USER/SESSION_USER for databases/tables etc. |
There was a problem hiding this comment.
Yeah, that's fine with me.
I don't know what the scope of that bug is, but if we don't fix it for 20.2, maybe we should just remove the syntax from the docs (everywhere, not just for schemas)?
Reviewed 6 of 6 files at r2.
Reviewable status:
v20.2/alter-schema.md, line 13 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
This definitely looks like a bug, yeah. Nice catch. I've filed cockroachdb/cockroach#54696 for investigation.
Thanks for looking into this.
v20.2/create-schema.md, line 108 at r2 (raw file):
~~~ ### Create two tables of the same name, in the same database
I find this a slightly confusing heading. Maybe "Create tables of the same name in different schemas"?
ericharmeling
force-pushed
the
user-defined-schemas
branch
from
3010ad9 to
4a7cc46
Compare
There was a problem hiding this comment.
Reviewable status:
v20.2/create-schema.md, line 108 at r2 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I find this a slightly confusing heading. Maybe "Create tables of the same name in different schemas"?
Done.
There was a problem hiding this comment.
Some nits but once fixed, LGTM!
Reviewable status:
v20.2/alter-schema.md, line 20 at r3 (raw file):
Parameter | Description ----------|------------ `RENAME TO ...` | Rename the schema.
Does the schema name need to be unique?
v20.2/alter-schema.md, line 21 at r3 (raw file):
----------|------------ `RENAME TO ...` | Rename the schema. `OWNER TO ...` | Change the owner of the schema. You can specify the new owner with a string literal or the [`CURRENT_USER` or `SESSION_USER` keywords](functions-and-operators.html#special-syntax-forms).
Possibly link to string.html here?
v20.2/alter-schema.md, line 123 at r3 (raw file):
~~~ Rename the schema as its owner:
Suggestion: reword to be something like As its owner, you can rename the schema:
v20.2/alter-schema.md, line 174 at r3 (raw file):
the current owner must belong to the role of the new owner (in this case, `max`)
Should the order on this be flipped since the new owner must be in the same role as the old owner? So: ...the new owner must belong to the role of the current owner
Also, should the parenthetical be (in this case, admin) since that's the role of the current owner? If that's the case, the next SQL statement should be GRANT max to admin right?
v20.2/create-schema.md, line 25 at r3 (raw file):
`IF NOT EXISTS` | Create a new schema only if a schema of the same name does not already exist. If one does exist, do not return an error. `schemaname` | The name of the schema to create, which must be unique and follow these [identifier rules](keywords-and-identifiers.html#identifiers). `AUTHORIZATION ...` | Optionally identify a user to be the owner of the schema. You can specify the owner with a string literal, or the [`CURRENT_USER` or `SESSION_USER` keywords](functions-and-operators.html#special-syntax-forms).<br><br>If a `CREATE SCHEMA` statement has an `AUTHORIZATION` clause, but no `schemaname`, the schema will be named after the specified owner of the schema. If a `CREATE SCHEMA` statement does not have an `AUTHORIZATION` clause, the user executing the statement will be named the owner.
Link to string.html?
v20.2/create-schema.md, line 275 at r3 (raw file):
~~~ `max` then inserts some values into the `accounts` table, without specifying a schema.
nit: end with a :
There was a problem hiding this comment.
TFTR @lnhsingh!
I addressed all of your comments, but my knowledge on a couple things might need verification from engineering.
Reviewable status:
v20.2/alter-schema.md, line 20 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Does the schema name need to be unique?
I think the new schema name must follow the same rules as the new table name in RENAME TABLE. Updated this description in a new commit.
@lucy-zhang
Does this look correct?
v20.2/alter-schema.md, line 21 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Possibly link to string.html here?
I'm not sure the STRING page will help here, since that page is about column data types rather than string literals. I linked instead to the wikipedia page for "String literal" (https://en.wikipedia.org/wiki/String_literal).
v20.2/alter-schema.md, line 123 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Suggestion: reword to be something like
As its owner, you can rename the schema:
Done.
v20.2/alter-schema.md, line 174 at r3 (raw file):
Should the order on this be flipped since the new owner must be in the same role as the old owner?
That would make sense, but I don't think that's technically how it's checked.
See the description of cockroachdb/cockroach#52781:
the following conditions must be met:
...
The user executing the command must be a member of the new owner role.
...
And in the logic tests at https://github.com/cockroachdb/cockroach/pull/52781/files#diff-1273a3998fbd62a94eafd18b2369954fR11 :
# Ensure the current user is a member of the role we're setting to.
statement error pq: must be member of role "testuser"
ALTER SCHEMA s OWNER TO testuser
The new owner (max) doesn't need any roles or privileges except CREATE on the database containing the schema. And the current user and current/old owner of the schema (root) needs to have the role of the new owner (max). Hence the need to grant the role max to root.
Also, should the parenthetical be
(in this case,admin)since that's the role of the current owner? If that's the case, the next SQL statement should beGRANT max to adminright?
It's the current user (root) that needs to be a member of the new owner's role (max). Since root is the current user, you need to grant max to root.
IIUC, users and roles are kind of the same thing (e.g., CREATE USER and CREATE ROLE are aliases). A user has privileges. That user can be operationalized as a "role". root is a member of the admin role, which just means that root has all of the privileges of the root user and all of the privileges of the admin user. When you grant a user the "role" of another user, you just copy those privileges over to that user. Very confusing haha. @arulajmani does that sounds correct?
v20.2/create-schema.md, line 25 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
Link to string.html?
Same response as above.
v20.2/create-schema.md, line 275 at r3 (raw file):
Previously, lnhsingh (Lauren Hirata Singh) wrote…
nit: end with a
:
Done.
There was a problem hiding this comment.
Deferring to @arulajmani for details on privileges.
Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Reviewable status:
v20.2/alter-schema.md, line 20 at r3 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
I think the new schema name must follow the same rules as the new table name in
RENAME TABLE. Updated this description in a new commit.@lucy-zhang
Does this look correct?
Unfortunately referring to schemas as database.schema doesn't work. It's only possible to refer to schemas in the current database. (This will likely be improved in 21.1.)
I think this is potentially confusing enough to users that it's worth highlighting (here and in all the new schema-related syntax). I should have mentioned that before.
v20.2/alter-schema.md, line 21 at r3 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
I'm not sure the
STRINGpage will help here, since that page is about column data types rather than string literals. I linked instead to the wikipedia page for "String literal" (https://en.wikipedia.org/wiki/String_literal).
I think this should just say You can specify the new owner by name or...
Same goes for the other usages.
v20.2/create-schema.md, line 108 at r5 (raw file):
~~~ ### Create two tables of the same name in the same schema
in different schemas
ericharmeling
force-pushed
the
user-defined-schemas
branch
from
d7473e4 to
665e95c
Compare
There was a problem hiding this comment.
Reviewable status:
v20.2/alter-schema.md, line 20 at r3 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
Unfortunately referring to schemas as
database.schemadoesn't work. It's only possible to refer to schemas in the current database. (This will likely be improved in 21.1.)I think this is potentially confusing enough to users that it's worth highlighting (here and in all the new schema-related syntax). I should have mentioned that before.
Thanks @lucy-zhang !
Updated ALTER SCHEMA, CREATE SCHEMA, and DROP SCHEMA pages.
v20.2/alter-schema.md, line 21 at r3 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I think this should just say
You can specify the new owner by name or...Same goes for the other usages.
Done!
v20.2/create-schema.md, line 108 at r5 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
in different schemas
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r6.
Reviewable status:
v20.2/create-schema.md, line 108 at r6 (raw file):
~~~ ### Create two tables of the same name in the different schemas
I don't think you need the
There was a problem hiding this comment.
Wait, I meant to approve it (aside from that nit).
Reviewable status:
ericharmeling
force-pushed
the
user-defined-schemas
branch
from
665e95c to
eca3a88
Compare
There was a problem hiding this comment.
Reviewable status:
v20.2/create-schema.md, line 108 at r6 (raw file):
Previously, lucy-zhang (Lucy Zhang) wrote…
I don't think you need
the
Agh! Fixed. Thanks!
|
If you've got no objections to the privileges-related docs here (see #8273 (review)), then I'm going to merge this. |
Fixes #8164.
Fixes #5993.
Fixes #8322.
Fixes #8346.
Fixes #8344.
Fixes #8305.
Fixes #8327.
CREATE SCHEMA,ALTER SCHEMA, andDROP SCHEMA.SHOW SCHEMASpage, and other references to user-defined schema support.Note that this PR pilots using CLI help text instead of SQL diagrams for statement syntax documentation.