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

diff --git a/doc/src/sgml/ref/comment.sgml b/doc/src/sgml/ref/comment.sgml
index bc848b306999abe7ebc1a917fdb52104c3adc9c7..1cdc49ff5fec1b5065d01820bb21837b9a22eff4 100644 (file)
--- a/doc/src/sgml/ref/comment.sgml
+++ b/doc/src/sgml/ref/comment.sgml
@@ -33,6 +33,7 @@ COMMENT ON
   DATABASE <replaceable class="PARAMETER">object_name</replaceable> |
   DOMAIN <replaceable class="PARAMETER">object_name</replaceable> |
   EXTENSION <replaceable class="PARAMETER">object_name</replaceable> |
+  FOREIGN DATA WRAPPER <replaceable class="PARAMETER">object_name</replaceable> |
   FOREIGN TABLE <replaceable class="PARAMETER">object_name</replaceable> |
   FUNCTION <replaceable class="PARAMETER">function_name</replaceable> ( [ [ <replaceable class="parameter">argmode</replaceable> ] [ <replaceable class="parameter">argname</replaceable> ] <replaceable class="parameter">argtype</replaceable> [, ...] ] ) |
   INDEX <replaceable class="PARAMETER">object_name</replaceable> |
@@ -45,6 +46,7 @@ COMMENT ON
   RULE <replaceable class="PARAMETER">rule_name</replaceable> ON <replaceable class="PARAMETER">table_name</replaceable> |
   SCHEMA <replaceable class="PARAMETER">object_name</replaceable> |
   SEQUENCE <replaceable class="PARAMETER">object_name</replaceable> |
+  SERVER <replaceable class="PARAMETER">object_name</replaceable> |
   TABLESPACE <replaceable class="PARAMETER">object_name</replaceable> |
   TEXT SEARCH CONFIGURATION <replaceable class="PARAMETER">object_name</replaceable> |
   TEXT SEARCH DICTIONARY <replaceable class="PARAMETER">object_name</replaceable> |

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 48fa6d48b7f49817ff3d87dc6fdddc818708bb19..aa3d59d4c9a31fbdebd07e1e62785f9f8d22d30d 100644 (file)
--- 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 880b95df0200905ec428de0a3333a6ae7414372c..0d21d310a650a3b1f01cc4c554e4e6ac06b63fba 100644 (file)
--- 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 acd40c1f4e9edc3a12644efc8f90c85b88806e2c..13d6d882f8852989163e04c759bf71410c4977a0 100644 (file)
--- 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 44cd18177c6f673469780f8a3fd9d215b43fac6d..cda90a6b0cb83d0626d13ffba06f57df66de81c6 100644 (file)
--- 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 27fdccae5b831fea616741a2d5000ceb2502ec9f..a22ab66ae5936c352686713ebb6c564872589576 100644 (file)
--- 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 ] <objname> |
+ *                                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 ] <objname> |
  *                              AGGREGATE <aggname> (arg1, ...) |
  *                              FUNCTION <funcname> (arg1, arg2, ...) |
  *                              OPERATOR <op> (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 691ba3bd95a510f37665d5ca187b5fdca232b020..2f27018b25648eaf68737b8ea5f08d49362f3d96 100644 (file)
--- 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 d676f3fce7451cbf880eb0c1c063a9250f20cb68..2fda9e39feb14fd4df2a7b45800c28d6d7ea9d0d 100644 (file)
--- 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 e96323efcc7e335c7797b28a1a4b43e86afcbc7b..b28b764fd528e976daba62efc46912ceb1467732 100644 (file)
--- 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 a7473349fdf0cad58f0a3ff8b015fee73eaf2550..c05bcabb71e32b1e321e1664a66babf6d730b9ed 100644 (file)
--- 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 3f3978590319583ca9fe173160c287d3655fdc87..0d12b98e2172e94e09947b977a7b41d6e4aaf532 100644 (file)
--- 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+