Re: ALTER TYPE OWNER fails to recurse to multirange
Robert Haas writes:
> On Tue, Jan 16, 2024 at 11:46 AM Tom Lane 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
Re: ALTER TYPE OWNER fails to recurse to multirange
On Tue, Jan 16, 2024 at 11:46 AM Tom Lane wrote: > They're by no means independent. What would it mean to have a > 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 > treat the multirange as dependent for some things: But this seems like an entirely valid point. -- Robert Haas EDB: http://www.enterprisedb.com
Re: ALTER TYPE OWNER fails to recurse to multirange
Robert Haas writes: > On Mon, Jan 15, 2024 at 2:28 PM Tom Lane wrote: >> 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. > 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. 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
Re: ALTER TYPE OWNER fails to recurse to multirange
On Mon, Jan 15, 2024 at 2:28 PM Tom Lane wrote: > 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. 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. -- Robert Haas EDB: http://www.enterprisedb.com
Re: ALTER TYPE OWNER fails to recurse to multirange
Robert Haas writes: > On Mon, Jan 15, 2024 at 1:27 PM Tom Lane wrote: >> 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. > 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'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 > 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. 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 > ownership and permissions on each separately? If ownership is a > problem, aren't permissions also? 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
Re: ALTER TYPE OWNER fails to recurse to multirange
On Mon, Jan 15, 2024 at 1:27 PM Tom Lane wrote: > 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. 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
ALTER TYPE OWNER fails to recurse to multirange
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
