Thanks for the review. On Mon, 4 Apr 2011 12:47:18 -0400 Robert Haas <robertmh...@gmail.com> wrote:
> On Mon, Apr 4, 2011 at 6:49 AM, Shigeru HANADA > <han...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers