sql: Implement grant and revoke privilege #18827
Conversation
jkosh44
force-pushed
the
grant-revoke-privilege
branch
4 times, most recently
from
062a26a
to
869dc0a
Compare
This commit implements the privilege variants of the SQL commands `GRANT` and `REVOKE`. Part of MaterializeInc#11579
jkosh44
force-pushed
the
grant-revoke-privilege
branch
from
869dc0a
to
cb6b23b
Compare
| @@ -723,6 +734,16 @@ pub enum DatabaseId { | |||
| System(u64), | |||
| } | |||
|
|
|||
| impl DatabaseId { | |||
| pub fn is_user(&self) -> bool { | |||
There was a problem hiding this comment.
This is unused, but it's fairly small and I think since we're adding is_system it's useful to have this around for the future.
| @@ -685,6 +686,16 @@ pub enum SchemaId { | |||
| System(u64), | |||
| } | |||
|
|
|||
| impl SchemaId { | |||
| pub fn is_user(&self) -> bool { | |||
| ); | ||
| existing_privilege.acl_mode = | ||
| existing_privilege.acl_mode.difference(privilege.acl_mode); | ||
| } |
There was a problem hiding this comment.
Don't need an else block here like above because it's not a problem to attempt to revoke a privilege that already doesn't exist?
There was a problem hiding this comment.
Yeah, that just ends up as a no-op.
| ObjectId::Cluster(cluster_id) | ObjectId::ClusterReplica((cluster_id, _)) => { | ||
| if cluster_id.is_system() { | ||
| let cluster = self.get_cluster(*cluster_id); | ||
| Err(Error::new(ErrorKind::ReadOnlyCluster( |
There was a problem hiding this comment.
Some of this logic is duplicated above in Op::GrantPrivilege. Should that call this function instead?
There was a problem hiding this comment.
To be honest, almost all of planning, sequencing, and the catalog commit are identical except for a few lines. I was tempted to implement Revoke and Grant as a single statement with a boolean flag/enum, to avoid duplication. That's actually how PostgreSQL implements Grant/Revoke.
What do you think? Is that going too far?
There was a problem hiding this comment.
Ok, I think I just massively cleaned this up in b04f54c
There was a problem hiding this comment.
Oh, I completely misunderstood this commit. This function is actually called in the sequencer, so I think it's safe to remove these checks from Op::GrantPrivilege.
This commit implements the privilege variants of the SQL commands
GRANTandREVOKE.Part of #11579
Motivation
This PR adds a known-desirable feature.
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.GRANTandREVOKEcommands for adding and removing object privileges.