Обсуждение: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.

On Fri, Apr 1, 2011 at 11:57 AM, Thom Brown <thom@linux.com> wrote:
> On 1 April 2011 16:28, Robert Haas <rhaas@postgresql.org> wrote:
>> Support comments on FOREIGN DATA WRAPPER and SERVER objects.
>>
>> This mostly involves making it work with the objectaddress.c framework,
>> which does most of the heavy lifting.  In that vein, change
>> GetForeignDataWrapperOidByName to get_foreign_data_wrapper_oid and
>> GetForeignServerOidByName to get_foreign_server_oid, to match the
>> pattern we use for other object types.
>
> Should we also have support for comments on user mappings?

Oh, bugger.  Yeah, probably.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/4/2 Robert Haas <robertmhaas@gmail.com>:
> On Fri, Apr 1, 2011 at 11:57 AM, Thom Brown <thom@linux.com> wrote:
>> Should we also have support for comments on user mappings?
>
> Oh, bugger.  Yeah, probably.

I'd work on this, if taking some days is OK.

Regards,
--
Shigeru Hanada

On Sun, 3 Apr 2011 10:51:04 +0900
Shigeru Hanada <shigeru.hanada@gmail.com> wrote:
> 2011/4/2 Robert Haas <robertmhaas@gmail.com>:
> > On Fri, Apr 1, 2011 at 11:57 AM, Thom Brown <thom@linux.com> wrote:
> >> Should we also have support for comments on user mappings?
> >
> > Oh, bugger.  Yeah, probably.
>
> I'd work on this, if taking some days is OK.

I've worked on this for a while and found some debatable points.

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?

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.

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.

Please see attached patches for details.
Sorry for long patch names, I generated these patches with git
format-patch.

And, attached test_user_mapping_comments.sql is a script which I've
used to test patches locally.

Regards,
--
Shigeru Hanada

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.

>> 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.  :-(

> 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Tue, 05 Apr 2011 13:37:48 +0900
Shigeru HANADA <hanada@metrosystems.co.jp> wrote:
> On Mon, 4 Apr 2011 12:47:18 -0400
> Robert Haas <robertmhaas@gmail.com> wrote:
> > 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.

Please find attached revised comment-on-user-mapping patches.

* The comment_user_mapping_core.patch includes syntax support, catalog
manipulation, pg_dump support, documents and regression tests.

Some functions were exposed to merge logic of user mapping owner check.

* The comment_user_mapping_psql.patch includes only psql
tab-completion feature.  It can be applied separately.

Regards,
--
Shigeru Hanada

On Tue, Apr 5, 2011 at 6:03 AM, Shigeru HANADA
<hanada@metrosystems.co.jp> wrote:
> On Tue, 05 Apr 2011 13:37:48 +0900
> Shigeru HANADA <hanada@metrosystems.co.jp> wrote:
>> On Mon, 4 Apr 2011 12:47:18 -0400
>> Robert Haas <robertmhaas@gmail.com> wrote:
>> > 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.
>
> Please find attached revised comment-on-user-mapping patches.
>
> * The comment_user_mapping_core.patch includes syntax support, catalog
> manipulation, pg_dump support, documents and regression tests.

I don't think it's going to fly to add a function
pg_usermapping_ownercheck() with a randomly different API than all the
parallel functions for other object types.  There is probably some
more refactoring that needs to be done here to make this sane, but I'm
coming around to the view that trying to slip this into 9.1 is not the
best thing for us to be spending time on, especially considering that
it doesn't seem to be straightforward to figure out how it should
actually work.  I am inclined to punt this to 9.2.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On mån, 2011-04-04 at 19:49 +0900, Shigeru HANADA wrote:
> 1) Who can comment on a user mapping?

I'm not sure that it's necessary to allow commenting on user mappings.
You can't comment on role grants either, for example.  They're somewhat
similar things.

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 5, 2011 at 6:03 AM, Shigeru HANADA
> <hanada@metrosystems.co.jp> wrote:
>> * The comment_user_mapping_core.patch includes syntax support, catalog
>> manipulation, pg_dump support, documents and regression tests.

> I don't think it's going to fly to add a function
> pg_usermapping_ownercheck() with a randomly different API than all the
> parallel functions for other object types.  There is probably some
> more refactoring that needs to be done here to make this sane, but I'm
> coming around to the view that trying to slip this into 9.1 is not the
> best thing for us to be spending time on, especially considering that
> it doesn't seem to be straightforward to figure out how it should
> actually work.  I am inclined to punt this to 9.2.

I agree --- this can clearly contains more worms than we expected.

Supporting user mappings in COMMENT, EXTENSION, etc is not so critical
that we should push a possibly misdesigned notion of ownership into
the system for it.  Better to take our time and think about that.

(BTW, it might be useful to reconsider casts while we are thinking about
this.  Those don't have a proper notion of ownership either.  I'm a bit
inclined to think that we should just bite the bullet and add owner
columns to both these catalogs.  But, again, let's not be hasty.)
        regards, tom lane

On tis, 2011-04-05 at 14:47 -0400, Tom Lane wrote:
> Supporting user mappings in COMMENT, EXTENSION, etc is not so critical
> that we should push a possibly misdesigned notion of ownership into
> the system for it.  Better to take our time and think about that.
> 
> (BTW, it might be useful to reconsider casts while we are thinking about
> this.  Those don't have a proper notion of ownership either.  I'm a bit
> inclined to think that we should just bite the bullet and add owner
> columns to both these catalogs.  But, again, let's not be hasty.)

As I said elsewhere, I think of user mappings as similar to role grants.
An owner there would be similar to a grantor, so it would make sense.