Adjust the permissions required for COMMENT ON ROLE.

Formerly, any member of a role could change the role's comment, as of
course could superusers; but holders of CREATEROLE privilege could not,
unless they were also members.  This led to the odd situation that a
CREATEROLE holder could create a role but then could not comment on it.
It also seems a bit dubious to let an unprivileged user change his own
comment, let alone those of group roles he belongs to.  So, change the
rule to be "you must be superuser to comment on a superuser role, or
hold CREATEROLE to comment on non-superuser roles".  This is the same
as the privilege check for creating/dropping roles, and thus fits much
better with the rule for other object types, namely that only the owner
of an object can comment on it.

In passing, clean up the documentation for COMMENT a little bit.

Per complaint from Owen Jacobson and subsequent discussion.

doc/src/sgml/func.sgml

@@ -13588,10 +13588,10 @@ SELECT typlen FROM pg_type WHERE oid = pg_typeof(33);
    </table>
 
    <para>
-    <function>col_description</function> returns the comment for a table column,
-    which is specified by the OID of its table and its column number.
-    <function>obj_description</function> cannot be used for table columns since
-    columns do not have OIDs of their own.
+    <function>col_description</function> returns the comment for a table
+    column, which is specified by the OID of its table and its column number.
+    (<function>obj_description</function> cannot be used for table columns
+    since columns do not have OIDs of their own.)
    </para>
 
    <para>
@@ -13610,8 +13610,8 @@ SELECT typlen FROM pg_type WHERE oid = pg_typeof(33);
     <function>shobj_description</function> is used just like
     <function>obj_description</function> except it is used for retrieving
     comments on shared objects.  Some system catalogs are global to all
-    databases within each cluster and their descriptions are stored globally
-    as well.
+    databases within each cluster, and the descriptions for objects in them
+    are stored globally as well.
    </para>
 
    <indexterm>

doc/src/sgml/ref/comment.sgml

@@ -65,11 +65,18 @@ COMMENT ON
   </para>
 
   <para>
-    To modify a comment, issue a new <command>COMMENT</> command for the
-    same object.  Only one comment string is stored for each object.
-    To remove a comment, write <literal>NULL</literal> in place of the text
-    string.
-    Comments are automatically dropped when the object is dropped.
+   Only one comment string is stored for each object, so to modify a comment,
+   issue a new <command>COMMENT</> command for the same object.  To remove a
+   comment, write <literal>NULL</literal> in place of the text string.
+   Comments are automatically dropped when their object is dropped.
+  </para>
+
+  <para>
+   For most kinds of object, only the object's owner can set the comment.
+   Roles don't have owners, so the rule for <literal>COMMENT ON ROLE</> is
+   that you must be superuser to comment on a superuser role, or have the
+   <literal>CREATEROLE</> privilege to comment on non-superuser roles.
+   Of course, a superuser can comment on anything.
   </para>
 
   <para>
@@ -93,15 +100,15 @@ COMMENT ON
     <term><replaceable class="parameter">agg_name</replaceable></term>
     <term><replaceable class="parameter">constraint_name</replaceable></term>
     <term><replaceable class="parameter">function_name</replaceable></term>
-    <term><replaceable class="parameter">op</replaceable></term>
+    <term><replaceable class="parameter">operator_name</replaceable></term>
     <term><replaceable class="parameter">rule_name</replaceable></term>
     <term><replaceable class="parameter">trigger_name</replaceable></term>
     <listitem>
      <para>
       The name of the object to be commented.  Names of tables,
-      aggregates, domains, foreign tables, functions, indexes, operators,
-      operator classes, operator families, sequences, text search objects,
-      types, and views can be schema-qualified.
+      aggregates, collations, conversions, domains, foreign tables, functions,
+      indexes, operators, operator classes, operator families, sequences,
+      text search objects, types, and views can be schema-qualified.
      </para>
     </listitem>
    </varlistentry>
@@ -137,7 +144,6 @@ COMMENT ON
 
    <varlistentry>
     <term><replaceable class="parameter">argmode</replaceable></term>
-
     <listitem>
      <para>
       The mode of a function argument: <literal>IN</>, <literal>OUT</>,
@@ -154,7 +160,6 @@ COMMENT ON
 
    <varlistentry>
     <term><replaceable class="parameter">argname</replaceable></term>
-
     <listitem>
      <para>
       The name of a function argument.
@@ -167,7 +172,6 @@ COMMENT ON
 
    <varlistentry>
     <term><replaceable class="parameter">argtype</replaceable></term>
-
     <listitem>
      <para>
       The data type(s) of the function's arguments (optionally
@@ -186,8 +190,19 @@ COMMENT ON
    </varlistentry>
 
    <varlistentry>
-     <term><literal>PROCEDURAL</literal></term>
+    <term><replaceable class="parameter">left_type</replaceable></term>
+    <term><replaceable class="parameter">right_type</replaceable></term>
+    <listitem>
+     <para>
+      The data type(s) of the operator's arguments (optionally
+      schema-qualified).  Write <literal>NONE</> for the missing argument
+      of a prefix or postfix operator.
+     </para>
+    </listitem>
+   </varlistentry>
 
+    <varlistentry>
+     <term><literal>PROCEDURAL</literal></term>
      <listitem>
       <para>
        This is a noise word.
@@ -212,12 +227,11 @@ COMMENT ON
   <title>Notes</title>
 
   <para>
-   There is presently no security mechanism for comments: any user
+   There is presently no security mechanism for viewing comments: any user
    connected to a database can see all the comments for objects in
-   that database (although only superusers can change comments for
-   objects that they don't own).  For shared objects such as
-   databases, roles, and tablespaces comments are stored globally
-   and any user connected to any database can see all the comments
+   that database.  For shared objects such as
+   databases, roles, and tablespaces, comments are stored globally so any
+   user connected to any database in the cluster can see all the comments
    for shared objects.  Therefore, don't put security-critical
    information in comments.
   </para>
@@ -257,7 +271,7 @@ COMMENT ON INDEX my_index IS 'Enforces uniqueness on employee ID';
 COMMENT ON LANGUAGE plpython IS 'Python support for stored procedures';
 COMMENT ON LARGE OBJECT 346344 IS 'Planning document';
 COMMENT ON OPERATOR ^ (text, text) IS 'Performs intersection of two texts';
-COMMENT ON OPERATOR - (NONE, text) IS 'This is a prefix operator on text';
+COMMENT ON OPERATOR - (NONE, integer) IS 'Unary minus';
 COMMENT ON OPERATOR CLASS int4ops USING btree IS '4 byte integer operators for btrees';
 COMMENT ON OPERATOR FAMILY integer_ops USING btree IS 'all integer operators for btrees';
 COMMENT ON ROLE my_role IS 'Administration group for finance tables';

src/backend/catalog/aclchk.c

@@ -4735,6 +4735,36 @@ pg_extension_ownercheck(Oid ext_oid, Oid roleid)
 	return has_privs_of_role(roleid, ownerId);
 }
 
+/*
+ * Check whether specified role has CREATEROLE privilege (or is a superuser)
+ *
+ * Note: roles do not have owners per se; instead we use this test in
+ * places where an ownership-like permissions test is needed for a role.
+ * Be sure to apply it to the role trying to do the operation, not the
+ * role being operated on!  Also note that this generally should not be
+ * considered enough privilege if the target role is a superuser.
+ * (We don't handle that consideration here because we want to give a
+ * separate error message for such cases, so the caller has to deal with it.)
+ */
+bool
+has_createrole_privilege(Oid roleid)
+{
+	bool		result = false;
+	HeapTuple	utup;
+
+	/* Superusers bypass all permission checking. */
+	if (superuser_arg(roleid))
+		return true;
+
+	utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(roleid));
+	if (HeapTupleIsValid(utup))
+	{
+		result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreaterole;
+		ReleaseSysCache(utup);
+	}
+	return result;
+}
+
 /*
  * Fetch pg_default_acl entry for given role, namespace and object type
  * (object type must be given in pg_default_acl's encoding).

src/backend/catalog/objectaddress.c

@@ -808,13 +808,6 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TABLESPACE,
 							   NameListToString(objname));
 			break;
-		case OBJECT_ROLE:
-			if (!has_privs_of_role(roleid, address.objectId))
-				ereport(ERROR,
-						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-						 errmsg("must be member of role \"%s\"",
-								NameListToString(objname))));
-			break;
 		case OBJECT_TSDICTIONARY:
 			if (!pg_ts_dict_ownercheck(address.objectId, roleid))
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TSDICTIONARY,
@@ -825,6 +818,26 @@ check_object_ownership(Oid roleid, ObjectType objtype, ObjectAddress address,
 				aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_TSCONFIGURATION,
 							   NameListToString(objname));
 			break;
+		case OBJECT_ROLE:
+			/*
+			 * We treat roles as being "owned" by those with CREATEROLE priv,
+			 * except that superusers are only owned by superusers.
+			 */
+			if (superuser_arg(address.objectId))
+			{
+				if (!superuser_arg(roleid))
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must be superuser")));
+			}
+			else
+			{
+				if (!has_createrole_privilege(roleid))
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must have CREATEROLE privilege")));
+			}
+			break;
 		case OBJECT_FDW:
 		case OBJECT_TSPARSER:
 		case OBJECT_TSTEMPLATE:

src/backend/commands/user.c

@@ -58,20 +58,7 @@ static void DelRoleMems(const char *rolename, Oid roleid,
 static bool
 have_createrole_privilege(void)
 {
-	bool		result = false;
-	HeapTuple	utup;
-
-	/* Superusers can always do everything */
-	if (superuser())
-		return true;
-
-	utup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId()));
-	if (HeapTupleIsValid(utup))
-	{
-		result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreaterole;
-		ReleaseSysCache(utup);
-	}
-	return result;
+	return has_createrole_privilege(GetUserId());
 }
 
 

src/include/utils/acl.h

@@ -317,5 +317,6 @@ 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_server_ownercheck(Oid srv_oid, Oid roleid);
 extern bool pg_extension_ownercheck(Oid ext_oid, Oid roleid);
+extern bool has_createrole_privilege(Oid roleid);
 
 #endif   /* ACL_H */