Re: Include RELKIND_TOASTVALUE in get_relkind_objtype - Mailing list pgsql-hackers

Michael Paquier <michael@paquier.xyz> writes:
> Okay.  Attached is what I was thinking about, with extra regression
> tests to cover the ground for toast tables and indexes that are able
> to reproduce the original failure, and more comments for the routines
> as they should be used only for ACL error messages.

I'd rather do something like the attached, which makes it more of an
explicit goal that we won't fail on bad input.  (As written, we'd only
fail on bad classId, which is a case that really shouldn't happen.)

Tests are the same as yours, but I revised the commentary and got
rid of the elog-for-bad-relkind.  I also made some cosmetic changes
in commands/alter.c, so as to (1) make it clear by inspection that
those calls are only used to feed aclcheck_error, and (2) avoid
uselessly computing a value that we won't need in normal non-error
cases.

            regards, tom lane

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index ce8a4e9..b8cbe6a 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2612,6 +2612,13 @@ get_object_attnum_acl(Oid class_id)
     return prop->attnum_acl;
 }

+/*
+ * get_object_type
+ *
+ * Return the object type associated with a given object.  This routine
+ * is primarily used to determine the object type to mention in ACL check
+ * error messages, so it's desirable for it to avoid failing.
+ */
 ObjectType
 get_object_type(Oid class_id, Oid object_id)
 {
@@ -5333,6 +5340,16 @@ strlist_to_textarray(List *list)
     return arr;
 }

+/*
+ * get_relkind_objtype
+ *
+ * Return the object type for the relkind given by the caller.
+ *
+ * If an unexpected relkind is passed, we say OBJECT_TABLE rather than
+ * failing.  That's because this is mostly used for generating error messages
+ * for failed ACL checks on relations, and we'd rather produce a generic
+ * message saying "table" than fail entirely.
+ */
 ObjectType
 get_relkind_objtype(char relkind)
 {
@@ -5352,13 +5369,10 @@ get_relkind_objtype(char relkind)
             return OBJECT_MATVIEW;
         case RELKIND_FOREIGN_TABLE:
             return OBJECT_FOREIGN_TABLE;
-
-            /*
-             * other relkinds are not supported here because they don't map to
-             * OBJECT_* values
-             */
+        case RELKIND_TOASTVALUE:
+            return OBJECT_TABLE;
         default:
-            elog(ERROR, "unexpected relkind: %d", relkind);
-            return 0;
+            /* Per above, don't raise an error */
+            return OBJECT_TABLE;
     }
 }
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 70dbcb0..562e3d3 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -172,7 +172,6 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
     AttrNumber    Anum_name = get_object_attnum_name(classId);
     AttrNumber    Anum_namespace = get_object_attnum_namespace(classId);
     AttrNumber    Anum_owner = get_object_attnum_owner(classId);
-    ObjectType    objtype = get_object_type(classId, objectId);
     HeapTuple    oldtup;
     HeapTuple    newtup;
     Datum        datum;
@@ -224,7 +223,8 @@ AlterObjectRename_internal(Relation rel, Oid objectId, const char *new_name)
         ownerId = DatumGetObjectId(datum);

         if (!has_privs_of_role(GetUserId(), DatumGetObjectId(ownerId)))
-            aclcheck_error(ACLCHECK_NOT_OWNER, objtype, old_name);
+            aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId),
+                           old_name);

         /* User must have CREATE privilege on the namespace */
         if (OidIsValid(namespaceId))
@@ -670,7 +670,6 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
     AttrNumber    Anum_name = get_object_attnum_name(classId);
     AttrNumber    Anum_namespace = get_object_attnum_namespace(classId);
     AttrNumber    Anum_owner = get_object_attnum_owner(classId);
-    ObjectType    objtype = get_object_type(classId, objid);
     Oid            oldNspOid;
     Datum        name,
                 namespace;
@@ -726,7 +725,7 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid)
         ownerId = DatumGetObjectId(owner);

         if (!has_privs_of_role(GetUserId(), ownerId))
-            aclcheck_error(ACLCHECK_NOT_OWNER, objtype,
+            aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objid),
                            NameStr(*(DatumGetName(name))));

         /* User must have CREATE privilege on new namespace */
@@ -950,8 +949,6 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
         /* Superusers can bypass permission checks */
         if (!superuser())
         {
-            ObjectType    objtype = get_object_type(classId, objectId);
-
             /* must be owner */
             if (!has_privs_of_role(GetUserId(), old_ownerId))
             {
@@ -970,7 +967,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId)
                     snprintf(namebuf, sizeof(namebuf), "%u", objectId);
                     objname = namebuf;
                 }
-                aclcheck_error(ACLCHECK_NOT_OWNER, objtype, objname);
+                aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId),
+                               objname);
             }
             /* Must be able to become new owner */
             check_is_member_of_role(GetUserId(), new_ownerId);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 3c2b166..1cdb7a9 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2434,8 +2434,17 @@ CREATE ROLE regress_reindexuser NOLOGIN;
 SET SESSION ROLE regress_reindexuser;
 REINDEX SCHEMA schema_to_reindex;
 ERROR:  must be owner of schema schema_to_reindex
+-- Permission failures with toast tables and indexes (pg_authid here)
+RESET ROLE;
+GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
+SET SESSION ROLE regress_reindexuser;
+REINDEX TABLE pg_toast.pg_toast_1260;
+ERROR:  must be owner of table pg_toast_1260
+REINDEX INDEX pg_toast.pg_toast_1260_index;
+ERROR:  must be owner of index pg_toast_1260_index
 -- Clean up
 RESET ROLE;
+REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
 DROP ROLE regress_reindexuser;
 DROP SCHEMA schema_to_reindex CASCADE;
 NOTICE:  drop cascades to 6 other objects
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index 26640f0..7659808 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -1003,8 +1003,15 @@ REINDEX SCHEMA CONCURRENTLY schema_to_reindex;
 CREATE ROLE regress_reindexuser NOLOGIN;
 SET SESSION ROLE regress_reindexuser;
 REINDEX SCHEMA schema_to_reindex;
+-- Permission failures with toast tables and indexes (pg_authid here)
+RESET ROLE;
+GRANT USAGE ON SCHEMA pg_toast TO regress_reindexuser;
+SET SESSION ROLE regress_reindexuser;
+REINDEX TABLE pg_toast.pg_toast_1260;
+REINDEX INDEX pg_toast.pg_toast_1260_index;

 -- Clean up
 RESET ROLE;
+REVOKE USAGE ON SCHEMA pg_toast FROM regress_reindexuser;
 DROP ROLE regress_reindexuser;
 DROP SCHEMA schema_to_reindex CASCADE;