I gone through patch and here is the review for this patch:
.) patch go applied on master branch with patch -p1 command
(git apply failed)
.) regression make check run fine
.) testcase coverage is missing in the patch
.) Over all coding/patch looks good.
Few comments:
1) Any particular reason for not adding SESSION_USER/USER also made usable
for this command ?
2) I think RoleId_or_curruser can be used for following role:
/* ALTER TABLE <name> OWNER TO RoleId */
| OWNER TO RoleId
3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
missing.
On Fri, Oct 10, 2014 at 1:57 PM, Kyotaro HORIGUCHI <
[email protected]> wrote:
> Hello, on the way considering alter role set .., I found that
> ALTER ROLE/USER cannot take CURRENT_USER as the role name.
>
> In addition to that, documents of ALTER ROLE / USER are
> inconsistent with each other in spite of ALTER USER is said to be
> an alias for ALTER ROLE. Plus, ALTER USER cannot take ALL as user
> name although ALTER ROLE can.
>
> This patch does following things,
>
> - ALTER USER/ROLE now takes CURRENT_USER as user name.
>
> - Rewrite sysnopsis of the documents for ALTER USER and ALTER
> ROLE so as to they have exactly same syntax.
>
> - Modify syntax of ALTER USER so as to be an alias of ALTER ROLE.
>
> - Added CURRENT_USER/CURRENT_ROLE syntax to both.
> - Added ALL syntax as user name to ALTER USER.
> - Added IN DATABASE syntax to ALTER USER.
>
> x Integrating ALTER ROLE/USER syntax could not be done because of
> shift/reduce error of bison.
>
> x This patch contains no additional regressions. Is it needed?
>
> SESSION_USER/USER also can be made usable for this command, but
> this patch doesn't so (yet).
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
> --
> Sent via pgsql-hackers mailing list ([email protected])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>
--
Rushabh Lathia
