Subject: ALTER TYPE OWNER fails to recurse to multirange

From: Tom Lane <[email-masked]>
Date: Mon, 15 Jan 2024 13:27:44 -0500

d=# create type varbitrange as range (subtype = varbit);
CREATE TYPE
d=# \dT+
                                             List of data types
 Schema |       Name       |  Internal name   | Size | Elements |  Owner   | Access privileges | Description 
--------+------------------+------------------+------+----------+----------+-------------------+-------------
 public | varbitmultirange | varbitmultirange | var  |          | postgres |                   | 
 public | varbitrange      | varbitrange      | var  |          | postgres |                   | 
(2 rows)

d=# create user joe;
CREATE ROLE
d=# alter type varbitrange owner to joe;
ALTER TYPE
d=# \dT+
                                             List of data types
 Schema |       Name       |  Internal name   | Size | Elements |  Owner   | Access privileges | Description 
--------+------------------+------------------+------+----------+----------+-------------------+-------------
 public | varbitmultirange | varbitmultirange | var  |          | postgres |                   | 
 public | varbitrange      | varbitrange      | var  |          | joe      |                   | 
(2 rows)

That's pretty broken, isn't it?  joe would own the multirange if he'd
created the range to start with.  Even if you think the ownerships
ideally should be separable, this behavior causes existing pg_dump
files to restore incorrectly, because pg_dump assumes it need not emit
any commands about the multirange.

A related issue is that you can manually alter the multirange's
ownership:

d=# alter type varbitmultirange owner to joe;
ALTER TYPE

which while it has some value in allowing recovery from this bug,
is inconsistent with our handling of other dependent types such
as arrays:

d=# alter type _varbitrange owner to joe;
ERROR:  cannot alter array type varbitrange[]
HINT:  You can alter type varbitrange, which will alter the array type as well.

Possibly the thing to do about that is to forbid it in HEAD
for consistency, while still allowing it in back branches
so that people can clean up inconsistent ownership if needed.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 15 Jan 2024 14:17:08 -0500

> any commands about the multirange.

I agree that pg_dump doing the wrong thing is bad, but the SQL example
doesn't look broken if you ignore pg_dump. I have a feeling that the
source of the awkwardness here is that one SQL command is creating two
objects, and unlike the case of a table and a TOAST table, one is not
an implementation detail of the other or clearly subordinate to the
other. But how does that prevent us from making pg_dump restore the
ownership and permissions on each separately? If ownership is a
problem, aren't permissions also?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Mon, 15 Jan 2024 14:28:31 -0500

> doesn't look broken if you ignore pg_dump.

I'm reasoning by analogy to array types, which are automatically
created and automatically updated to keep the same ownership
etc. properties as their base type.  To the extent that multirange
types don't act exactly like that, I say it's a bug/oversight in the
multirange patch.  So I think this is a backend bug, not a pg_dump
bug.

> I have a feeling that the
> other.

How is a multirange not subordinate to the underlying range type?
It can't exist without it, and we automatically create it without
any further information when you make the range type.  That smells
a lot like the way we handle array types.  The array behavior is of
very long standing and surprises nobody.

> But how does that prevent us from making pg_dump restore the

Probably, and I wouldn't be surprised if we've also failed to make
multiranges follow arrays in the permissions department.  An
array type can't have an ACL of its own, IIRC.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 15 Jan 2024 15:01:25 -0500

> multirange patch.  So I think this is a backend bug, not a pg_dump

Oh...

Well, I guess maybe I'm just clueless. I thought that the range and
multirange were two essentially independent objects being created by
the same command. But I haven't studied the implementation so maybe
I'm completely wrong.

---

From: Tom Lane <[email-masked]>
Date: Tue, 16 Jan 2024 11:46:45 -0500

> the same command. But I haven't studied the implementation so maybe

They're by no means independent.  What would it mean to have a
multirange without the underlying range type?  Also, we already
treat the multirange as dependent for some things:

d=# create type varbitrange as range (subtype = varbit);
CREATE TYPE
d=# \dT
           List of data types
 Schema |       Name       | Description 
--------+------------------+-------------
 public | varbitmultirange | 
 public | varbitrange      | 
(2 rows)

d=# drop type varbitmultirange;
ERROR:  cannot drop type varbitmultirange because type varbitrange requires it
HINT:  You can drop type varbitrange instead.
d=# drop type varbitrange restrict;
DROP TYPE
d=# \dT
     List of data types
 Schema | Name | Description 
--------+------+-------------
(0 rows)

So I think we're looking at a half-baked dependency design,
not two independent objects.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Tue, 16 Jan 2024 12:06:43 -0500

On Tue, Jan 16, 2024 at 11:46 AM Tom Lane <[email-masked]> wrote:
> multirange without the underlying range type?

It would mean just that - no more, and no less. If it's possible to
imagine a data type that stores pairs of values from the underlying
data type with the constraint that the first is less than the second,
plus the ability to specify inclusive or exclusive bounds and the
ability to have infinite bounds, then it's equally possible to imagine
a data type that represents a set of such ranges such that no two
ranges in the set overlap. And you need not imagine that the former
data type must exist in order for the latter to exist. Theoretically,
they're just two different data types that somebody could decide to
create.

> Also, we already

But this seems like an entirely valid point.

---

From: Tom Lane <[email-masked]>
Date: Mon, 12 Feb 2024 17:55:13 -0500

> 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
 --