Kyotaro HORIGUCHI wrote:
> Hello, thank you for the comment.
>
> > Looking at this a bit, I'm not sure completely replacing RoleId with a
> > node is the best idea; some of the users of that production in the
> > grammar are okay with accepting only normal strings as names, and don't
> > need all the CURRENT_USER etc stuff.
>
> You're right. the productions don't need RoleSpec already uses
> char* for the role member in its *Stmt structs. I might have done
> a bit too much.
>
> Adding new nonterminal RoleId and using it makes a bit duplicate
> of check code for "public"/"none" and others but removes other
> redundant check for "!= CSTRING" from some production, CREATE
> ROLE/USER/GROUP, ALTER ROLE/USER/GROUP RENAME.
Thanks for doing the fiddly work here. Attached is a new version of
this patch. I simplified some things, including removing those rules
you added to RoleId. It seems to me that this problem:
> RoleId in the patch still has rule components for CURRENT_USER,
> SESSION_USER, and CURRENT_ROLE. Without them, the parser prints
> an error ununderstandable to users.
>
> | =# alter role current_user rename to "PuBlic";
> | ERROR: syntax error at or near "rename"
> | LINE 1: alter role current_user rename to "PuBlic";
> | ^
can be fixed without complicating the rest of the stuff simply by using
RoleSpec instead of RoleId and doing the error checks at the RenameStmt
production. Altering the current user and session user is disallowed
downstream, so there's no reason we can't just have gram.y throw the
same error when special node types are used; so we end up passing the
role name only (just as currently) and the error message remains clear.
I couldn't find any further problems with this version of the code,
though I also noticed that a lot of things are not being tested in the
regression tests, such as "create user public" or "alter user none". It
would be good to have tests for such cases, to avoid breaking them
accidentally. If you can spare some time to submit test cases for such
commands, I would be thankful.
I'm pretty sure, thought I haven't tried yet, that we can now remove the
PrivGrantee node completely.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
