From 50533a6dc515cc3182f52838275c9d2a1f587604 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Fri, 1 Apr 2011 11:28:28 -0400 Subject: [PATCH] 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 --- 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(-) diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml index bc848b3069..1cdc49ff5f 100644 --- a/doc/src/sgml/ref/comment.sgml +++ b/doc/src/sgml/ref/comment.sgml @@ -33,6 +33,7 @@ COMMENT ON DATABASE object_name | DOMAIN object_name | EXTENSION object_name | + FOREIGN DATA WRAPPER object_name | FOREIGN TABLE object_name | FUNCTION function_name ( [ [ argmode ] [ argname ] argtype [, ...] ] ) | INDEX object_name | @@ -45,6 +46,7 @@ COMMENT ON RULE rule_name ON table_name | SCHEMA object_name | SEQUENCE object_name | + SERVER object_name | TABLESPACE object_name | TEXT SEARCH CONFIGURATION object_name | TEXT SEARCH DICTIONARY object_name | diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 48fa6d48b7..aa3d59d4c9 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -683,7 +683,7 @@ objectNamesToOids(GrantObjectType objtype, List *objnames) foreach(cell, objnames) { char *fdwname = strVal(lfirst(cell)); - Oid fdwid = GetForeignDataWrapperOidByName(fdwname, false); + Oid fdwid = get_foreign_data_wrapper_oid(fdwname, false); objects = lappend_oid(objects, fdwid); } @@ -692,7 +692,7 @@ objectNamesToOids(GrantObjectType objtype, List *objnames) foreach(cell, objnames) { char *srvname = strVal(lfirst(cell)); - Oid srvid = GetForeignServerOidByName(srvname, false); + Oid srvid = get_foreign_server_oid(srvname, false); objects = lappend_oid(objects, srvid); } @@ -4588,6 +4588,33 @@ pg_ts_config_ownercheck(Oid cfg_oid, Oid roleid) return has_privs_of_role(roleid, ownerId); } +/* + * Ownership check for a foreign-data wrapper (specified by OID). + */ +bool +pg_foreign_data_wrapper_ownercheck(Oid srv_oid, Oid roleid) +{ + HeapTuple tuple; + Oid ownerId; + + /* Superusers bypass all permission checking. */ + if (superuser_arg(roleid)) + return true; + + tuple = SearchSysCache1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(srv_oid)); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("foreign-data wrapper with OID %u does not exist", + srv_oid))); + + ownerId = ((Form_pg_foreign_data_wrapper) GETSTRUCT(tuple))->fdwowner; + + ReleaseSysCache(tuple); + + return has_privs_of_role(roleid, ownerId); +} + /* * Ownership check for a foreign server (specified by OID). */ diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index 880b95df02..0d21d310a6 100644 --- a/src/backend/catalog/objectaddress.c +++ b/src/backend/catalog/objectaddress.c @@ -30,6 +30,8 @@ #include "catalog/pg_conversion.h" #include "catalog/pg_database.h" #include "catalog/pg_extension.h" +#include "catalog/pg_foreign_data_wrapper.h" +#include "catalog/pg_foreign_server.h" #include "catalog/pg_language.h" #include "catalog/pg_largeobject.h" #include "catalog/pg_largeobject_metadata.h" @@ -52,6 +54,7 @@ #include "commands/proclang.h" #include "commands/tablespace.h" #include "commands/trigger.h" +#include "foreign/foreign.h" #include "libpq/be-fsstubs.h" #include "miscadmin.h" #include "nodes/makefuncs.h" @@ -140,6 +143,8 @@ get_object_address(ObjectType objtype, List *objname, List *objargs, case OBJECT_ROLE: case OBJECT_SCHEMA: case OBJECT_LANGUAGE: + case OBJECT_FDW: + case OBJECT_FOREIGN_SERVER: address = get_object_address_unqualified(objtype, objname); break; case OBJECT_TYPE: @@ -295,6 +300,12 @@ get_object_address_unqualified(ObjectType objtype, List *qualname) case OBJECT_LANGUAGE: msg = gettext_noop("language name cannot be qualified"); break; + case OBJECT_FDW: + msg = gettext_noop("foreign-data wrapper name cannot be qualified"); + break; + case OBJECT_FOREIGN_SERVER: + msg = gettext_noop("server name cannot be qualified"); + break; default: elog(ERROR, "unrecognized objtype: %d", (int) objtype); msg = NULL; /* placate compiler */ @@ -340,6 +351,16 @@ get_object_address_unqualified(ObjectType objtype, List *qualname) address.objectId = get_language_oid(name, false); address.objectSubId = 0; break; + case OBJECT_FDW: + address.classId = ForeignDataWrapperRelationId; + address.objectId = get_foreign_data_wrapper_oid(name, false); + address.objectSubId = 0; + break; + case OBJECT_FOREIGN_SERVER: + address.classId = ForeignServerRelationId; + address.objectId = get_foreign_server_oid(name, false); + address.objectSubId = 0; + break; default: elog(ERROR, "unrecognized objtype: %d", (int) objtype); /* placate compiler, which doesn't know elog won't return */ @@ -655,6 +676,12 @@ object_exists(ObjectAddress address) case CastRelationId: indexoid = CastOidIndexId; break; + case ForeignDataWrapperRelationId: + cache = FOREIGNDATAWRAPPEROID; + break; + case ForeignServerRelationId: + cache = FOREIGNSERVEROID; + break; case TSParserRelationId: cache = TSPARSEROID; break; @@ -758,6 +785,11 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address, aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_EXTENSION, NameListToString(objname)); break; + case OBJECT_FDW: + if (!pg_foreign_data_wrapper_ownercheck(address.objectId, roleid)) + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_FDW, + NameListToString(objname)); + break; case OBJECT_FOREIGN_SERVER: if (!pg_foreign_server_ownercheck(address.objectId, roleid)) aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_FOREIGN_SERVER, @@ -838,7 +870,6 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address, errmsg("must have CREATEROLE privilege"))); } break; - case OBJECT_FDW: case OBJECT_TSPARSER: case OBJECT_TSTEMPLATE: /* We treat these object types as being owned by superusers */ diff --git a/src/backend/commands/foreigncmds.c b/src/backend/commands/foreigncmds.c index acd40c1f4e..13d6d882f8 100644 --- a/src/backend/commands/foreigncmds.c +++ b/src/backend/commands/foreigncmds.c @@ -686,7 +686,7 @@ RemoveForeignDataWrapper(DropFdwStmt *stmt) Oid fdwId; ObjectAddress object; - fdwId = GetForeignDataWrapperOidByName(stmt->fdwname, true); + fdwId = get_foreign_data_wrapper_oid(stmt->fdwname, true); if (!superuser()) ereport(ERROR, @@ -959,7 +959,7 @@ RemoveForeignServer(DropForeignServerStmt *stmt) Oid srvId; ObjectAddress object; - srvId = GetForeignServerOidByName(stmt->servername, true); + srvId = get_foreign_server_oid(stmt->servername, true); if (!OidIsValid(srvId)) { diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 44cd18177c..cda90a6b0c 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -79,26 +79,6 @@ GetForeignDataWrapper(Oid fdwid) } -/* - * GetForeignDataWrapperOidByName - look up the foreign-data wrapper - * OID by name. - */ -Oid -GetForeignDataWrapperOidByName(const char *fdwname, bool missing_ok) -{ - Oid fdwId; - - fdwId = GetSysCacheOid1(FOREIGNDATAWRAPPERNAME, CStringGetDatum(fdwname)); - - if (!OidIsValid(fdwId) && !missing_ok) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("foreign-data wrapper \"%s\" does not exist", - fdwname))); - - return fdwId; -} - /* * GetForeignDataWrapperByName - look up the foreign-data wrapper @@ -107,7 +87,7 @@ GetForeignDataWrapperOidByName(const char *fdwname, bool missing_ok) ForeignDataWrapper * GetForeignDataWrapperByName(const char *fdwname, bool missing_ok) { - Oid fdwId = GetForeignDataWrapperOidByName(fdwname, missing_ok); + Oid fdwId = get_foreign_data_wrapper_oid(fdwname, missing_ok); if (!OidIsValid(fdwId)) return NULL; @@ -171,32 +151,13 @@ GetForeignServer(Oid serverid) } -/* - * GetForeignServerByName - look up the foreign server oid by name. - */ -Oid -GetForeignServerOidByName(const char *srvname, bool missing_ok) -{ - Oid serverid; - - serverid = GetSysCacheOid1(FOREIGNSERVERNAME, CStringGetDatum(srvname)); - - if (!OidIsValid(serverid) && !missing_ok) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("server \"%s\" does not exist", srvname))); - - return serverid; -} - - /* * GetForeignServerByName - look up the foreign server definition by name. */ ForeignServer * GetForeignServerByName(const char *srvname, bool missing_ok) { - Oid serverid = GetForeignServerOidByName(srvname, missing_ok); + Oid serverid = get_foreign_server_oid(srvname, missing_ok); if (!OidIsValid(serverid)) return NULL; @@ -538,3 +499,42 @@ postgresql_fdw_validator(PG_FUNCTION_ARGS) PG_RETURN_BOOL(true); } + +/* + * get_foreign_data_wrapper_oid - given a FDW name, look up the OID + * + * If missing_ok is false, throw an error if name not found. If true, just + * return InvalidOid. + */ +Oid +get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok) +{ + Oid oid; + + oid = GetSysCacheOid1(FOREIGNDATAWRAPPERNAME, CStringGetDatum(fdwname)); + if (!OidIsValid(oid) && !missing_ok) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("foreign-data wrapper \"%s\" does not exist", + fdwname))); + return oid; +} + +/* + * get_foreign_server_oid - given a FDW name, look up the OID + * + * If missing_ok is false, throw an error if name not found. If true, just + * return InvalidOid. + */ +Oid +get_foreign_server_oid(const char *servername, bool missing_ok) +{ + Oid oid; + + oid = GetSysCacheOid1(FOREIGNSERVERNAME, CStringGetDatum(servername)); + if (!OidIsValid(oid) && !missing_ok) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("server \"%s\" does not exist", servername))); + return oid; +} diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 27fdccae5b..a22ab66ae5 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -4787,11 +4787,12 @@ opt_restart_seqs: * the object associated with the comment. The form of the statement is: * * COMMENT ON [ [ DATABASE | DOMAIN | INDEX | SEQUENCE | TABLE | TYPE | VIEW | - * COLLATION | CONVERSION | LANGUAGE | OPERATOR CLASS | LARGE OBJECT | - * CAST | COLUMN | SCHEMA | TABLESPACE | EXTENSION | ROLE | - * TEXT SEARCH PARSER | TEXT SEARCH DICTIONARY | - * TEXT SEARCH TEMPLATE | TEXT SEARCH CONFIGURATION | - * FOREIGN TABLE ] | + * COLLATION | CONVERSION | LANGUAGE | OPERATOR CLASS | + * LARGE OBJECT | CAST | COLUMN | SCHEMA | TABLESPACE | + * EXTENSION | ROLE | TEXT SEARCH PARSER | + * TEXT SEARCH DICTIONARY | TEXT SEARCH TEMPLATE | + * TEXT SEARCH CONFIGURATION | FOREIGN TABLE | + * FOREIGN DATA WRAPPER | SERVER ] | * AGGREGATE (arg1, ...) | * FUNCTION (arg1, arg2, ...) | * OPERATOR (leftoperand_typ, rightoperand_typ) | @@ -4971,6 +4972,8 @@ comment_type: | EXTENSION { $$ = OBJECT_EXTENSION; } | ROLE { $$ = OBJECT_ROLE; } | FOREIGN TABLE { $$ = OBJECT_FOREIGN_TABLE; } + | SERVER { $$ = OBJECT_FOREIGN_SERVER; } + | FOREIGN DATA_P WRAPPER { $$ = OBJECT_FDW; } ; comment_text: diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 691ba3bd95..2f27018b25 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -3162,7 +3162,7 @@ convert_foreign_data_wrapper_name(text *fdwname) { char *fdwstr = text_to_cstring(fdwname); - return GetForeignDataWrapperOidByName(fdwstr, false); + return get_foreign_data_wrapper_oid(fdwstr, false); } /* @@ -3928,7 +3928,7 @@ convert_server_name(text *servername) { char *serverstr = text_to_cstring(servername); - return GetForeignServerOidByName(serverstr, false); + return get_foreign_server_oid(serverstr, false); } /* diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index d676f3fce7..2fda9e39fe 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -70,12 +70,13 @@ typedef struct ForeignTable extern ForeignServer *GetForeignServer(Oid serverid); extern ForeignServer *GetForeignServerByName(const char *name, bool missing_ok); -extern Oid GetForeignServerOidByName(const char *name, bool missing_ok); extern UserMapping *GetUserMapping(Oid userid, Oid serverid); extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid); extern ForeignDataWrapper *GetForeignDataWrapperByName(const char *name, bool missing_ok); -extern Oid GetForeignDataWrapperOidByName(const char *name, bool missing_ok); extern ForeignTable *GetForeignTable(Oid relid); +extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok); +extern Oid get_foreign_server_oid(const char *servername, bool missing_ok); + #endif /* FOREIGN_H */ diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h index e96323efcc..b28b764fd5 100644 --- a/src/include/utils/acl.h +++ b/src/include/utils/acl.h @@ -315,6 +315,7 @@ extern bool pg_collation_ownercheck(Oid coll_oid, Oid roleid); extern bool pg_conversion_ownercheck(Oid conv_oid, Oid roleid); extern bool pg_ts_dict_ownercheck(Oid dict_oid, Oid roleid); extern bool pg_ts_config_ownercheck(Oid cfg_oid, Oid roleid); +extern bool pg_foreign_data_wrapper_ownercheck(Oid srv_oid, Oid roleid); extern bool pg_foreign_server_ownercheck(Oid srv_oid, Oid roleid); extern bool pg_extension_ownercheck(Oid ext_oid, Oid roleid); extern bool has_createrole_privilege(Oid roleid); diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index a7473349fd..c05bcabb71 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -14,6 +14,7 @@ CREATE ROLE regress_test_role_super SUPERUSER; CREATE ROLE regress_test_indirect; CREATE ROLE unprivileged_role; CREATE FOREIGN DATA WRAPPER dummy; +COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless'; CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator; -- At this point we should have 2 built-in wrappers and no servers. SELECT fdwname, fdwhandler::regproc, fdwvalidator::regproc, fdwoptions FROM pg_foreign_data_wrapper ORDER BY 1, 2, 3; @@ -211,6 +212,7 @@ DROP ROLE regress_test_role_super; CREATE FOREIGN DATA WRAPPER foo; CREATE SERVER s1 FOREIGN DATA WRAPPER foo; +COMMENT ON SERVER s1 IS 'foreign server'; CREATE USER MAPPING FOR current_user SERVER s1; \dew+ List of foreign-data wrappers diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql index 3f39785903..0d12b98e21 100644 --- a/src/test/regress/sql/foreign_data.sql +++ b/src/test/regress/sql/foreign_data.sql @@ -21,6 +21,7 @@ CREATE ROLE regress_test_indirect; CREATE ROLE unprivileged_role; CREATE FOREIGN DATA WRAPPER dummy; +COMMENT ON FOREIGN DATA WRAPPER dummy IS 'useless'; CREATE FOREIGN DATA WRAPPER postgresql VALIDATOR postgresql_fdw_validator; -- At this point we should have 2 built-in wrappers and no servers. @@ -99,6 +100,7 @@ DROP ROLE regress_test_role_super; CREATE FOREIGN DATA WRAPPER foo; CREATE SERVER s1 FOREIGN DATA WRAPPER foo; +COMMENT ON SERVER s1 IS 'foreign server'; CREATE USER MAPPING FOR current_user SERVER s1; \dew+ \des+