-
Notifications
You must be signed in to change notification settings - Fork 636
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
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
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".
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.