Robert Haas <robertmh...@gmail.com> writes:
> On Tue, Jan 16, 2024 at 11:46 AM Tom Lane <t...@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
--
