On Fri, Oct 04, 2019 at 05:55:40PM +0900, Michael Paquier wrote: > On Thu, Oct 03, 2019 at 09:52:34AM -0400, Tom Lane wrote: >> FWIW, I really dislike this patch, mainly because it is based on the >> assumption (as John said) that get_relkind_objtype is used only >> in aclcheck_error calls. However it's not obvious why that should >> be true, and there certainly is no documentation suggesting that >> it needs to be true. That's mainly because get_relkind_objtype has no >> documentation period, which if you ask me is flat out unacceptable >> for a globally-exposed function. (Same comment about its wrapper >> get_object_type.) > > Yes, I agree that the expectations that the caller of this function > can have are hard to guess. So we could tackle this occasion to add > more comments. I could try to come up with a better patch. Or > perhaps you have already your mind on it?
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. Any thoughts? -- Michael
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index ce8a4e927d..b3fdee476b 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 to a given object. This routine
+ * should be used to generate proper error messages for ACL check failures
+ * on relations.
+ */
ObjectType
get_object_type(Oid class_id, Oid object_id)
{
@@ -5333,6 +5340,13 @@ strlist_to_textarray(List *list)
return arr;
}
+/*
+ * get_relkind_objtype
+ *
+ * Return the object type equivalent to the relkind defined by the caller.
+ * This routine should be used to generate error messages for failed
+ * ACL checks on relations with details about the object type.
+ */
ObjectType
get_relkind_objtype(char relkind)
{
@@ -5353,6 +5367,14 @@ get_relkind_objtype(char relkind)
case RELKIND_FOREIGN_TABLE:
return OBJECT_FOREIGN_TABLE;
+ /*
+ * Toast tables are not mapped with any ObjectType enum values,
+ * still OBJECT_TABLE is used as an equivalent here to not create
+ * confusing error messages as handled below.
+ */
+ case RELKIND_TOASTVALUE:
+ return OBJECT_TABLE;
+
/*
* other relkinds are not supported here because they don't map to
* OBJECT_* values
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 324db1b6ae..757a4eb8e3 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2308,8 +2308,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 f96bebf410..38324a77d5 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -949,8 +949,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;
signature.asc
Description: PGP signature
