-
Notifications
You must be signed in to change notification settings - Fork 665
Add PostgreSQL CREATE USER and ALTER USER support
#2015
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
base: main
Are you sure you want to change the base?
Conversation
7c91833 to
d38c2d0
Compare
Currently, the library only supports `CREATE ROLE` and `ALTER ROLE` for PostgreSQL. `CREATE USER` and `ALTER USER` fail to parse with errors like `"Expected: an object type after CREATE, found: USER"` But in PostgreSQL reference: - `CREATE USER` is equivalent to `CREATE ROLE`, except that `LOGIN` is assumed by default - `ALTER USER` is an alias to `ALTER ROLE` - Both should support the same options as their ROLE counterparts This commit extends the existing `CreateRole` and `AlterRole` structures to distinct which keyword has been used: `USER` or `ROLE`. It allows these expressions to be parsed and displayed back.
d38c2d0 to
51621b5
Compare
| /// Whether ROLE or USER keyword was used | ||
| keyword: RoleKeyword, |
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.
Could we use something like this representation instead? I think its a somewhat more invasive changes but clearer API wise.
struct CreateUserOrRole {
names: Vec<ObjectName>,
if_not_exists: bool,
// Postgres
login: Option<bool>,
inherit: Option<bool>,
// ...
}
Statement::CreateUser(CreateUserOrRole)
Statement::CreateRole(CreateUserOrRole)
struct AlterUserOrRole {
name: Ident
}
Statement::AlterUser(AlterUserOrRole)
Statement::AlterRole(AlterUserOrRole)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.
We have a namespace problem here, because there's already a CreateUser structure for Snowflake semantics. In the grand scheme of things, CreateUser and CreateRole should probably be both compatible with both Snowflake and PostgreSQL? I'm not sure if I'm seasoned enough with the codebase to bring that in, and that would be a very different kind of work than this PR. :)
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.
Ah I'm not sure I followed, do we mean snowflake has CreateUser supported by the parser already?
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.
Yes, but as far as I understand, it's a very different syntax / semantics. Postgres "USER" is really just an alias for "ROLE", and Postgres semantics are currently handled by CreateRole and AlterRole. Which is why I just added the keyword here: it doesn't introduce any backwards compatibility. It's just more data to the AST for an already existing expression parsing.
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.
Again, this would probably need a refactor so that all dialects use the same structures for ALTER / CREATE USER / ROLE, but I guess it would make more sense in a second step – the inconsistency is already there. This PR fixes a currently broken expression parsing "for free".
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.
Sorry for the delay in getting back to this. I think ideally we would use separate representations, it would likely be confusing if a CREATE USER statement was being parsed as an CREATE ROLE statement or vice versa by the parser.
Also heads up there is a related PR adding ALTER USER here
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.
From what I see of that PR, it is modeled on the current CreateUser structure, which only supports Snowflake.
So my understanding is that we'll have CreateUser and AlterUser that work for Snowflake, CreateRole and AlterRole that work for PostgreSQL, and no support for the opposite keyword / technology combo (CREATE USER support on PostgreSQL, or CREATE ROLE support on Snowflake).
I'd be happy to see the four structures unified into something that work for both technologies but I don't think I can do that at the moment, alas. :(
97de846 to
1118ae4
Compare
Currently, the library only supports
CREATE ROLEandALTER ROLEforPostgreSQL.
CREATE USERandALTER USERfail to parse with errorslike
"Expected: an object type after CREATE, found: USER"But in PostgreSQL reference:
CREATE USERis equivalent toCREATE ROLE, except thatLOGINis assumed by defaultALTER USERis an alias toALTER ROLEThis commit extends the existing
CreateRoleandAlterRolestructures to distinct which keyword has been used:
USERorROLE. It allows these expressions to be parsed and displayed back.