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

Author: robertmhaas

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

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).
  */

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 */

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))
 	{

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;
+}

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:

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);
 }
 
 /*

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 */

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);

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

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+