Re: ALTER TYPE OWNER fails to recurse to multirange - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jan 16, 2024 at 11:46 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Also, we already
>> treat the multirange as dependent for some things:

> But this seems like an entirely valid point.

Yeah, it's a bit of a muddle.  But there is no syntax for making
a standalone multirange type, so it seems to me that we've mostly
determined that multiranges are dependent types.  There are just
a few places that didn't get the word.

Attached is a proposed patch to enforce that ownership and permissions
of a multirange are those of the underlying range type, in ways
parallel to how we treat array types.  This is all that I found by
looking for calls to IsTrueArrayType().  It's possible that there's
some dependent-type behavior somewhere that isn't conditioned on that,
but I can't think of a good way to search.

If we don't do this, then we need some hacking in pg_dump to get it
to save and restore these properties of multiranges, so it's not like
the status quo is acceptable.

I'd initially thought that perhaps we could back-patch parts of this,
but now I'm not sure; it seems like these behaviors are a bit
intertwined.  Given the lack of field complaints I'm inclined to leave
things alone in the back branches.

            regards, tom lane

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 590affb79a..591e767cce 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -2447,11 +2447,17 @@ ExecGrant_Type_check(InternalGrant *istmt, HeapTuple tuple)

     pg_type_tuple = (Form_pg_type) GETSTRUCT(tuple);

+    /* Disallow GRANT on dependent types */
     if (IsTrueArrayType(pg_type_tuple))
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_GRANT_OPERATION),
                  errmsg("cannot set privileges of array types"),
                  errhint("Set the privileges of the element type instead.")));
+    if (pg_type_tuple->typtype == TYPTYPE_MULTIRANGE)
+        ereport(ERROR,
+                (errcode(ERRCODE_INVALID_GRANT_OPERATION),
+                 errmsg("cannot set privileges of multirange types"),
+                 errhint("Set the privileges of the range type instead.")));

     /* Used GRANT DOMAIN on a non-domain? */
     if (istmt->objtype == OBJECT_DOMAIN &&
@@ -3806,6 +3812,35 @@ pg_type_aclmask_ext(Oid type_oid, Oid roleid, AclMode mask, AclMaskHow how,
         typeForm = (Form_pg_type) GETSTRUCT(tuple);
     }

+    /*
+     * Likewise, multirange types don't manage their own permissions; consult
+     * the associated range type.  (Note we must do this after the array step
+     * to get the right answer for arrays of multiranges.)
+     */
+    if (typeForm->typtype == TYPTYPE_MULTIRANGE)
+    {
+        Oid            rangetype = get_multirange_range(type_oid);
+
+        ReleaseSysCache(tuple);
+
+        tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(rangetype));
+        if (!HeapTupleIsValid(tuple))
+        {
+            if (is_missing != NULL)
+            {
+                /* return "no privileges" instead of throwing an error */
+                *is_missing = true;
+                return 0;
+            }
+            else
+                ereport(ERROR,
+                        (errcode(ERRCODE_UNDEFINED_OBJECT),
+                         errmsg("type with OID %u does not exist",
+                                rangetype)));
+        }
+        typeForm = (Form_pg_type) GETSTRUCT(tuple);
+    }
+
     /*
      * Now get the type's owner and ACL from the tuple
      */
diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
index d7167108fb..fe47be38d0 100644
--- a/src/backend/catalog/pg_type.c
+++ b/src/backend/catalog/pg_type.c
@@ -326,14 +326,15 @@ TypeCreate(Oid newTypeOid,
                  errmsg("fixed-size types must have storage PLAIN")));

     /*
-     * This is a dependent type if it's an implicitly-created array type, or
-     * if it's a relation rowtype that's not a composite type.  For such types
-     * we'll leave the ACL empty, and we'll skip creating some dependency
-     * records because there will be a dependency already through the
-     * depended-on type or relation.  (Caution: this is closely intertwined
-     * with some behavior in GenerateTypeDependencies.)
+     * This is a dependent type if it's an implicitly-created array type or
+     * multirange type, or if it's a relation rowtype that's not a composite
+     * type.  For such types we'll leave the ACL empty, and we'll skip
+     * creating some dependency records because there will be a dependency
+     * already through the depended-on type or relation.  (Caution: this is
+     * closely intertwined with some behavior in GenerateTypeDependencies.)
      */
     isDependentType = isImplicitArray ||
+        typeType == TYPTYPE_MULTIRANGE ||
         (OidIsValid(relationOid) && relationKind != RELKIND_COMPOSITE_TYPE);

     /*
@@ -534,8 +535,9 @@ TypeCreate(Oid newTypeOid,
  * relationKind and isImplicitArray are likewise somewhat expensive to deduce
  * from the tuple, so we make callers pass those (they're not optional).
  *
- * isDependentType is true if this is an implicit array or relation rowtype;
- * that means it doesn't need its own dependencies on owner etc.
+ * isDependentType is true if this is an implicit array, multirange, or
+ * relation rowtype; that means it doesn't need its own dependencies on owner
+ * etc.
  *
  * We make an extension-membership dependency if we're in an extension
  * script and makeExtensionDep is true (and isDependentType isn't true).
@@ -601,18 +603,23 @@ GenerateTypeDependencies(HeapTuple typeTuple,
      * Make dependencies on namespace, owner, ACL, extension.
      *
      * Skip these for a dependent type, since it will have such dependencies
-     * indirectly through its depended-on type or relation.
+     * indirectly through its depended-on type or relation.  An exception is
+     * that multiranges need their own namespace dependency, since we don't
+     * force them to be in the same schema as their range type.
      */

-    /* placeholder for all normal dependencies */
+    /* collects normal dependencies for bulk recording */
     addrs_normal = new_object_addresses();

-    if (!isDependentType)
+    if (!isDependentType || typeForm->typtype == TYPTYPE_MULTIRANGE)
     {
         ObjectAddressSet(referenced, NamespaceRelationId,
                          typeForm->typnamespace);
-        recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
+        add_exact_object_address(&referenced, addrs_normal);
+    }

+    if (!isDependentType)
+    {
         recordDependencyOnOwner(TypeRelationId, typeObjectId,
                                 typeForm->typowner);

@@ -727,6 +734,16 @@ GenerateTypeDependencies(HeapTuple typeTuple,
         recordDependencyOn(&myself, &referenced,
                            isImplicitArray ? DEPENDENCY_INTERNAL : DEPENDENCY_NORMAL);
     }
+
+    /*
+     * Note: you might expect that we should record an internal dependency of
+     * a multirange on its range type here, by analogy with the cases above.
+     * But instead, that is done by RangeCreate(), which also handles
+     * recording of other range-type-specific dependencies.  That's pretty
+     * bogus.  It's okay for now, because there are no cases where we need to
+     * regenerate the dependencies of a range or multirange type.  But someday
+     * we might need to move that logic here to allow such regeneration.
+     */
 }

 /*
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index a400fb39f6..e0275e5fe9 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -3647,6 +3647,8 @@ RenameType(RenameStmt *stmt)
                  errhint("You can alter type %s, which will alter the array type as well.",
                          format_type_be(typTup->typelem))));

+    /* we do allow separate renaming of multirange types, though */
+
     /*
      * If type is composite we need to rename associated pg_class entry too.
      * RenameRelationInternal will call RenameTypeInternal automatically.
@@ -3730,6 +3732,21 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
                  errhint("You can alter type %s, which will alter the array type as well.",
                          format_type_be(typTup->typelem))));

+    /* don't allow direct alteration of multirange types, either */
+    if (typTup->typtype == TYPTYPE_MULTIRANGE)
+    {
+        Oid            rangetype = get_multirange_range(typeOid);
+
+        /* We don't expect get_multirange_range to fail, but cope if so */
+        ereport(ERROR,
+                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                 errmsg("cannot alter multirange type %s",
+                        format_type_be(typeOid)),
+                 OidIsValid(rangetype) ?
+                 errhint("You can alter type %s, which will alter the multirange type as well.",
+                         format_type_be(rangetype)) : 0));
+    }
+
     /*
      * If the new owner is the same as the existing owner, consider the
      * command to have succeeded.  This is for dump restoration purposes.
@@ -3769,13 +3786,13 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
 /*
  * AlterTypeOwner_oid - change type owner unconditionally
  *
- * This function recurses to handle a pg_class entry, if necessary.  It
- * invokes any necessary access object hooks.  If hasDependEntry is true, this
- * function modifies the pg_shdepend entry appropriately (this should be
- * passed as false only for table rowtypes and array types).
+ * This function recurses to handle dependent types (arrays and multiranges).
+ * It invokes any necessary access object hooks.  If hasDependEntry is true,
+ * this function modifies the pg_shdepend entry appropriately (this should be
+ * passed as false only for table rowtypes and dependent types).
  *
  * This is used by ALTER TABLE/TYPE OWNER commands, as well as by REASSIGN
- * OWNED BY.  It assumes the caller has done all needed check.
+ * OWNED BY.  It assumes the caller has done all needed checks.
  */
 void
 AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
@@ -3815,7 +3832,7 @@ AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
  * AlterTypeOwnerInternal - bare-bones type owner change.
  *
  * This routine simply modifies the owner of a pg_type entry, and recurses
- * to handle a possible array type.
+ * to handle any dependent types.
  */
 void
 AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
@@ -3865,6 +3882,19 @@ AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
     if (OidIsValid(typTup->typarray))
         AlterTypeOwnerInternal(typTup->typarray, newOwnerId);

+    /* If it is a range type, update the associated multirange too */
+    if (typTup->typtype == TYPTYPE_RANGE)
+    {
+        Oid            multirange_typeid = get_range_multirange(typeOid);
+
+        if (!OidIsValid(multirange_typeid))
+            ereport(ERROR,
+                    (errcode(ERRCODE_UNDEFINED_OBJECT),
+                     errmsg("could not find multirange type for data type %s",
+                            format_type_be(typeOid))));
+        AlterTypeOwnerInternal(multirange_typeid, newOwnerId);
+    }
+
     /* Clean up */
     table_close(rel, RowExclusiveLock);
 }
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 348748bae5..f40bc759c5 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -1868,7 +1868,7 @@ selectDumpableType(TypeInfo *tyinfo, Archive *fout)
         return;
     }

-    /* skip auto-generated array types */
+    /* skip auto-generated array and multirange types */
     if (tyinfo->isArray || tyinfo->isMultirange)
     {
         tyinfo->dobj.objType = DO_DUMMY_TYPE;
@@ -1876,8 +1876,8 @@ selectDumpableType(TypeInfo *tyinfo, Archive *fout)
         /*
          * Fall through to set the dump flag; we assume that the subsequent
          * rules will do the same thing as they would for the array's base
-         * type.  (We cannot reliably look up the base type here, since
-         * getTypes may not have processed it yet.)
+         * type or multirange's range type.  (We cannot reliably look up the
+         * base type here, since getTypes may not have processed it yet.)
          */
     }

@@ -5770,7 +5770,7 @@ getTypes(Archive *fout, int *numTypes)
         else
             tyinfo[i].isArray = false;

-        if (tyinfo[i].typtype == 'm')
+        if (tyinfo[i].typtype == TYPTYPE_MULTIRANGE)
             tyinfo[i].isMultirange = true;
         else
             tyinfo[i].isMultirange = false;
diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out
index 6d9498cdd1..5a9ee5d2cd 100644
--- a/src/test/regress/expected/dependency.out
+++ b/src/test/regress/expected/dependency.out
@@ -144,7 +144,6 @@ owner of sequence deptest_a_seq
 owner of table deptest
 owner of function deptest_func()
 owner of type deptest_enum
-owner of type deptest_multirange
 owner of type deptest_range
 owner of table deptest2
 owner of sequence ss1
diff --git a/src/test/regress/expected/multirangetypes.out b/src/test/regress/expected/multirangetypes.out
index 9808587532..002775b9eb 100644
--- a/src/test/regress/expected/multirangetypes.out
+++ b/src/test/regress/expected/multirangetypes.out
@@ -3115,6 +3115,36 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
 drop type textrange1;
 drop type textrange2;
 --
+-- Multiranges don't have their own ownership or permissions.
+--
+create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
+create role regress_multirange_owner;
+alter type multitextrange1 owner to regress_multirange_owner;  -- fail
+ERROR:  cannot alter multirange type multitextrange1
+HINT:  You can alter type textrange1, which will alter the multirange type as well.
+alter type textrange1 owner to regress_multirange_owner;
+set role regress_multirange_owner;
+revoke usage on type multitextrange1 from public;  -- fail
+ERROR:  cannot set privileges of multirange types
+HINT:  Set the privileges of the range type instead.
+revoke usage on type textrange1 from public;
+\dT+ *textrange1*
+                                                                     List of data types
+ Schema |      Name       |  Internal name  | Size | Elements |          Owner           |                  Access
privileges                 | Description  

+--------+-----------------+-----------------+------+----------+--------------------------+-----------------------------------------------------+-------------
+ public | multitextrange1 | multitextrange1 | var  |          | regress_multirange_owner |
                       |  
+ public | textrange1      | textrange1      | var  |          | regress_multirange_owner |
regress_multirange_owner=U/regress_multirange_owner|  
+(2 rows)
+
+create temp table test1(f1 multitextrange1);
+revoke usage on type textrange1 from regress_multirange_owner;
+create temp table test2(f1 multitextrange1);  -- fail
+ERROR:  permission denied for type multitextrange1
+drop table test1;
+drop type textrange1;
+reset role;
+drop role regress_multirange_owner;
+--
 -- Test polymorphic type system
 --
 create function anyarray_anymultirange_func(a anyarray, r anymultirange)
diff --git a/src/test/regress/sql/multirangetypes.sql b/src/test/regress/sql/multirangetypes.sql
index cadf312031..197e3af29c 100644
--- a/src/test/regress/sql/multirangetypes.sql
+++ b/src/test/regress/sql/multirangetypes.sql
@@ -700,6 +700,27 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
 drop type textrange1;
 drop type textrange2;

+--
+-- Multiranges don't have their own ownership or permissions.
+--
+create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
+create role regress_multirange_owner;
+
+alter type multitextrange1 owner to regress_multirange_owner;  -- fail
+alter type textrange1 owner to regress_multirange_owner;
+set role regress_multirange_owner;
+revoke usage on type multitextrange1 from public;  -- fail
+revoke usage on type textrange1 from public;
+\dT+ *textrange1*
+create temp table test1(f1 multitextrange1);
+revoke usage on type textrange1 from regress_multirange_owner;
+create temp table test2(f1 multitextrange1);  -- fail
+
+drop table test1;
+drop type textrange1;
+reset role;
+drop role regress_multirange_owner;
+
 --
 -- Test polymorphic type system
 --