Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.

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