Thanks for the review.
On Mon, 4 Apr 2011 12:47:18 -0400
Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Apr 4, 2011 at 6:49 AM, Shigeru HANADA
> <hanada@metrosystems.co.jp> wrote:
> > 1) Who can comment on a user mapping?
> > Basically only the owner can comment on a object, but user mappings
> > don't have owner. So following rules for ALTER/DROP seems good
> > because they are similarly allowed to only owner. In addition to
> > server's owner, a user can perform ALTER/DROP USER MAPPING if target
> > mapping is his own user's and USAGE privilege on the server has been
> > granted. This means that mappings for PUBLIC can be commented by only
> > server's owner. Is this spec reasonable?
>
> I guess so. The existing rules for user mapping permissions are not
> really what I would have expected, but there doesn't seem to be any
> particular reason to deviate from them here. I do think however that
> we should try NOT to duplicate the logic in user_mapping_ddl_aclcheck
> into multiple places, as someone will certainly screw that up down the
> road. If we're going to have complex logic like this we need to at
> least make sure that we only have one copy of it.
Agreed, I'm going to merge them.
> >> 2) How to specify user name of the target mapping
> > ALTER/DROP USER MAPPING also accept USER and CURRENT_USER as current
> > user. This syntax seems suitable for COMMENT ON USER MAPPING too for
> > consistency and usability.
>
> OK, sounds fine. But what does it actually mean when you say
> {ALTER|DROP|COMMENT ON} USER MAPPING FOR USER SERVER servername? I
> see some code to handle CURRENT_USER, but I must be missing the
> special-casing for USER. It's not documented, either. :-(
The statement above also operates a user mapping which was created for
current user explicitly because both USER and CURRENT_USER in the
context of authentication identifier have same meaning.
The parser transforms them into a C-lang-string "current_user" in
auth_ident of gram.y, so only "current_user" is handled in code.
Agreed with that this behavior should be documented in where
"current_user" is handled in backend code.
> > 3) Omitting ACL framework support
> > ISTM that full-support of ACL framework is not necessary for USER
> > MAPPING because USER MAPPING has no GRANT/REVOKE statements.
> > COMMENT ON USER MAPPING patch works fine, but some oversight might be
> > here.
>
> I agree.
>
> BTW, I think you can merge patches 0001 to 0004 into a single patch.
They were separated just for review, so I'll post revised and unified
patch ASAP.
Regards,
--
Shigeru Hanada