From 3e8235ba4f9cc3375b061fb5d3f3575434539b5f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 14 Feb 2024 11:30:39 -0500 Subject: [PATCH] Fix multiranges to behave more like dependent types. 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 | 35 ++++++++++++++++++++++ src/backend/catalog/pg_type.c | 41 ++++++++++++++++++-------- src/backend/commands/typecmds.c | 42 +++++++++++++++++++++++---- src/bin/pg_dump/pg_dump.c | 8 ++--- src/test/regress/expected/dependency.out | 1 - src/test/regress/expected/multirangetypes.out | 30 +++++++++++++++++++ src/test/regress/sql/multirangetypes.sql | 21 ++++++++++++++ 7 files changed, 155 insertions(+), 23 deletions(-) diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 590affb79a..1e44a71f61 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 && @@ -3807,6 +3813,35 @@ pg_type_aclmask_ext(Oid type_oid, Oid roleid, AclMode mask, AclMaskHow how, } /* + * 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 */ ownerId = typeForm->typowner; 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..c6363ebeb2 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..41d5524285 100644 --- a/src/test/regress/sql/multirangetypes.sql +++ b/src/test/regress/sql/multirangetypes.sql @@ -701,6 +701,27 @@ 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 -- -- 2.11.4.GIT