Fix multiranges to behave more like dependent types.
authorTom Lane <[email protected]>
Wed, 14 Feb 2024 16:30:39 +0000 (11:30 -0500)
committerTom Lane <[email protected]>
Wed, 14 Feb 2024 16:30:39 +0000 (11:30 -0500)
For most purposes, multiranges act like dependent objects of the
associated range type: you can't create them separately or drop them
separately.  This is like the way that autogenerated array types
behave.  However, a couple of points were overlooked: array types
automatically track the ownership of their base type, and array types
do not have their own permissions but use those of the base type,
while multiranges didn't emulate those behaviors.  This is fairly
broken, mainly because pg_dump doesn't think it needs to worry about
multiranges as separate objects, and thus it fails to dump/restore
ownership or permissions of multiranges.

There's no apparent value in letting a multirange diverge from
its parent's ownership or permissions, so let's make them act like
arrays in these respects.  However, we continue to let multiranges
be renamed or moved to a different schema independently of their
parent, since that doesn't break anything.

Discussion: https://postgr.es/m/1580383.1705343264@sss.pgh.pa.us

src/backend/catalog/aclchk.c
src/backend/catalog/pg_type.c
src/backend/commands/typecmds.c
src/bin/pg_dump/pg_dump.c
src/test/regress/expected/dependency.out
src/test/regress/expected/multirangetypes.out
src/test/regress/sql/multirangetypes.sql

index 590affb79a582bc168e7f7b89e7be9f8372f3c80..1e44a71f61c5007611e2457dd06f1e91e9da4fc9 100644 (file)
@@ -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(typeForm->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
     */
index d7167108fb1ee89cb2fe540885ae91da84675656..fe47be38d0e39fb6aa1511ed7d1d170da82a4987 100644 (file)
@@ -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.
+    */
 }
 
 /*
index a400fb39f67bf91f9c62bcdc80126d9e0cab1e1a..e0275e5fe9c185e7cf6628e3a938de30fb96b841 100644 (file)
@@ -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);
 }
index 348748bae538fad615cd5ffde132b08bc94acbd4..f40bc759c5c81053e93e30e8fb9d180578fc30a4 100644 (file)
@@ -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;
index 6d9498cdd1844ab15bc3fdaba85cdfb347e3c4f3..5a9ee5d2cd4f3adf1a398903ffb911e652ddefd5 100644 (file)
@@ -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
index 9808587532cdb2a56e9906779e361ed98d2d2c53..c6363ebeb24ca3c99c56afa719cc24771e1df5b6 100644 (file)
@@ -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)
index cadf312031ff95df793fcada3e55621dbf60ca0b..41d5524285a39fd3d24b0e59895088f1397f2ccc 100644 (file)
@@ -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
 --