Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Support comments on FOREIGN DATA WRAPPER and SERVER objects.
- 50533a6dc515 9.1.0 cited
-
pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Robert Haas <rhaas@postgresql.org> — 2011-04-01T15:28:34Z
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. Robert Haas and Shigeru Hanada Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/50533a6dc515cc3182f52838275c9d2a1f587604 Modified Files -------------- doc/src/sgml/ref/comment.sgml | 2 + src/backend/catalog/aclchk.c | 31 ++++++++++- src/backend/catalog/objectaddress.c | 33 +++++++++++- src/backend/commands/foreigncmds.c | 4 +- src/backend/foreign/foreign.c | 82 ++++++++++++++-------------- src/backend/parser/gram.y | 13 +++-- src/backend/utils/adt/acl.c | 4 +- src/include/foreign/foreign.h | 5 +- src/include/utils/acl.h | 1 + src/test/regress/expected/foreign_data.out | 2 + src/test/regress/sql/foreign_data.sql | 2 + 11 files changed, 124 insertions(+), 55 deletions(-)
-
Re: pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Thom Brown <thom@linux.com> — 2011-04-01T15:57:11Z
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. > > Robert Haas and Shigeru Hanada > > Branch > ------ > master > > Details > ------- > http://git.postgresql.org/pg/commitdiff/50533a6dc515cc3182f52838275c9d2a1f587604 > > Modified Files > -------------- > doc/src/sgml/ref/comment.sgml | 2 + > src/backend/catalog/aclchk.c | 31 ++++++++++- > src/backend/catalog/objectaddress.c | 33 +++++++++++- > src/backend/commands/foreigncmds.c | 4 +- > src/backend/foreign/foreign.c | 82 ++++++++++++++-------------- > src/backend/parser/gram.y | 13 +++-- > src/backend/utils/adt/acl.c | 4 +- > src/include/foreign/foreign.h | 5 +- > src/include/utils/acl.h | 1 + > src/test/regress/expected/foreign_data.out | 2 + > src/test/regress/sql/foreign_data.sql | 2 + > 11 files changed, 124 insertions(+), 55 deletions(-) Should we also have support for comments on user mappings? -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Robert Haas <robertmhaas@gmail.com> — 2011-04-01T17:37:58Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Shigeru Hanada <shigeru.hanada@gmail.com> — 2011-04-03T01:51:04Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
花田 茂 <hanada@metrosystems.co.jp> — 2011-04-04T10:49:09Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Robert Haas <robertmhaas@gmail.com> — 2011-04-04T16:47:18Z
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 -
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
花田 茂 <hanada@metrosystems.co.jp> — 2011-04-05T04:37:48Z
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 -
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
花田 茂 <hanada@metrosystems.co.jp> — 2011-04-05T10:03:07Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Robert Haas <robertmhaas@gmail.com> — 2011-04-05T16:25:42Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Peter Eisentraut <peter_e@gmx.net> — 2011-04-05T17:44:12Z
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.
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Tom Lane <tgl@sss.pgh.pa.us> — 2011-04-05T18:47:51Z
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
-
Re: Re: [COMMITTERS] pgsql: Support comments on FOREIGN DATA WRAPPER and SERVER objects.
Peter Eisentraut <peter_e@gmx.net> — 2011-04-05T21:04:21Z
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.