Thank you for reviewing,
2014 13:10:57 +0530, Rushabh Lathia <rushabh.lathia@gmail.com> wrote in
<CAGPqQf0kDFAJiZx0vCA_-wAZwU+Xj5MDNL-HGg1SEz9AW3ck7w@mail.gmail.com>
> 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 ?
No particular reson. This patch was the first step and if this is
the adequate way and adding them is desirable, I will add them.
> 2) I think RoleId_or_curruser can be used for following role:
>
> /* ALTER TABLE <name> OWNER TO RoleId */
> | OWNER TO RoleId
Good catch. I'll try it.
> 3) In the documentation patch, added for CURRENT_USER but CURRENT_ROLE is
> missing.
Mmm.. I didn't added CURRENT_ROLE in the previous patch.
I suppose CURRENT_ROLE is an implicit alias of CURRENT_USER
because it is not explained in the page below in spite of
existing in syntax.
http://www.postgresql.org/docs/9.4/static/functions-info.html
and it is currently usable only in expressions, so the following
SQL failed and all syntax using auth_ident will fail.
postgres=# CREATE USER MAPPING FOR CURRENT_ROLE SERVER s1;
ERROR: syntax error at or near "current_role"
share/doc/html/sql-createusermapping.html
| Synopsis
|
| CREATE USER MAPPING FOR { user_name | USER | CURRENT_USER | PUBLIC }
I don't know why the 'USER' is added here, but anyway I can add
'CURRENT_ROLE' there in gram.y but it seems not necessary to add
it to document.
Ok, I'll modify this patch so that,
- Make CURRENT_USER/ROLE usable in TABLE OWNER TO.
and since adding CURRENT_ROLE is the another thing, I'll do the
following things as additional patch.
- Add USER, CURRENT_ROLE and SESSION_* for the place where CURRENT_USER is usable now. auth_ident and
RoleId_or_curruser.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center