Re: alter user/role CURRENT_USER

All,

I just ran through the patch giving it a good once over, some items to
address/consider/discuss:

* Trailing whitespace.
* Why are you making changes in foreigncmds.c? These seem like unnecessary
changes. I see that you are trying to consolidate but this refactor seems
potentially out of scope.
* To the above point, is ResolveRoleId really necessary? Also, I'm not
sure I agree with passing in the tuple, I don't see any reason to pull that
look up into this function. Also, couldn't you simply just add a short
circuit in "get_role_oid" to check for "current_user" and return
GetUserId() and similar for "session_user"?
* In gram.y, is there really a need to have a separate RoleId_or_curruser?
Why not:

-RoleId: NonReservedWord { $$ = $1; };
+RoleId: CURRENT_USER { $$ =
"current_user";}
+ | USER { $$ = "current_user";}
+ | CURRENT_ROLE { $$ = "current_user";}
+ | SESSION_USER { $$ = "session_user";}
+ | NonReservedWord { $$ = $1; }
+ ;

This would certainly reduce the number of changes to the grammar but might
also help with covering the cases mentioned by Rushabh? Also are there
cases when we don't want CURRENT_USER to be considered a RoleId?

* The following seems like an unnecessary change:

- | RoleId { $$ = (strcmp($1, "public") == 0) ? NULL : $1;
}
+ RoleId { $$ = (strcmp($1, "public") == 0) ?
+ NULL : $1; }

* Why is htup.h included in dbcommands.h?
* What's up with the following in user.c, did you replace tab with spaces?

- HeapTuple roletuple;
+ HeapTuple roletuple;

-- Not working
> alter default privileges for role current_user grant SELECT ON TABLES TO
> current_user ;
>
-- Not working
> grant user1 TO current_user;
>

Agreed. The latter of the two seems like an interesting case to me
though. But they both kind of jump out at me as potential for security
concerns (but perhaps not given the role id checks, etc). At any rate, I'd
still expect these to behave accordingly with notification/error messages
when appropriate.

Their might few more syntax like this.
>

I think this can be "covered" inherently by the grammar changes recommended
above (if such changes are appropriate). Though, I'd also recommend
investigating which commands are affected via the grammar (or RoleId) and
then making sure to update the regression tests and the documentation
accordingly.

> I understand that patch is sightly getting bigger and complex then what
> it was
> originally proposed.
>

IMHO, I think this patch has become more complex than is required.

> Before going back to more review on latest patch I would
> like to understand the requirement of this new feature. Also would like
> others
> to comment on where/how we should restrict this feature ?
>

I think this is a fair request. I believe I can see the potential
convenience of these changes, however, I'm uncertain of the necessity of
them and whether or not it opens any security concerns (again, perhaps not,
but I think it is worth asking). Also, how would this affect items like
auditing?

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com