Kyotaro,
Food for thought. Couldn't you reduce the following block:
+ if (strcmp(stmt->role, "current_user") == 0)
+ {
+ roleid = GetUserId();
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("roleid %d does not exist", roleid)));
+ }
+ else
+ {
+ tuple = SearchSysCache1(AUTHNAME, PointerGetDatum(stmt->role));
+ if (!HeapTupleIsValid(tuple))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("role \"%s\" does not exist", stmt->role)));
To:
if (strcmp(stmt->role, "current_user") == 0)
roleid = GetUserId();
else
roleid = get_role_oid(stmt->role, false);
tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("roleid %d does not exist", roleid)));
I think this makes it a bit cleaner. It also reuses existing code as
'get_role_oid()' already does a valid role name check and will raise the
proper error.
-Adam
On Mon, Oct 20, 2014 at 3:40 AM, Rushabh Lathia <[email protected]>
wrote:
> 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
>
--
Adam Brightwell - [email protected]
Database Engineer - www.crunchydatasolutions.com
