8000 Fix multiranges to behave more like dependent types. · postgrespro/postgres@3e8235b · GitHub
[go: up one dir, main page]
Skip to content

    • Commit 3e8235b

    Browse files
    tglsfdctglsfdc
    committed
    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
    1 parent bd8fc16 commit 3e8235b

    File tree

    7 files changed

    +155
    -23
    lines changed

    7 files changed

    +155
    -23
    lines changed

    ‎src/backend/catalog/aclchk.c

    Lines changed: 35 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -2447,11 +2447,17 @@ ExecGrant_Type_check(InternalGrant *istmt, HeapTuple tuple)
    24472447

    24482448
    pg_type_tuple = (Form_pg_type) GETSTRUCT(tuple);
    24492449

    2450+
    /* Disallow GRANT on dependent types */
    24502451
    if (IsTrueArrayType(pg_type_tuple))
    24512452
    ereport(ERROR,
    24522453
    (errcode(ERRCODE_INVALID_GRANT_OPERATION),
    24532454
    errmsg("cannot set privileges of array types"),
    24542455
    errhint("Set the privileges of the element type instead.")));
    2456+
    if (pg_type_tuple->typtype == TYPTYPE_MULTIRANGE)
    2457+
    ereport(ERROR,
    2458+
    (errcode(ERRCODE_INVALID_GRANT_OPERATION),
    2459+
    errmsg("cannot set privileges of multirange types"),
    2460+
    errhint("Set the privileges of the range type instead.")));
    24552461

    24562462
    /* Used GRANT DOMAIN on a non-domain? */
    24572463
    if (istmt->objtype == OBJECT_DOMAIN &&
    @@ -3806,6 +3812,35 @@ pg_type_aclmask_ext(Oid type_oid, Oid roleid, AclMode mask, AclMaskHow how,
    38063812
    typeForm = (Form_pg_type) GETSTRUCT(tuple);
    38073813
    }
    38083814

    3815+
    /*
    3816+
    * Likewise, multirange types don't manage their own permissions; consult
    3817+
    * the associated range type. (Note we must do this after the array step
    3818+
    * to get the right answer for arrays of multiranges.)
    3819+
    */
    3820+
    if (typeForm->typtype == TYPTYPE_MULTIRANGE)
    3821+
    {
    3822+
    Oid rangetype = get_multirange_range(typeForm->oid);
    3823+
    3824+
    ReleaseSysCache(tuple);
    3825+
    3826+
    tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(rangetype));
    3827+
    if (!HeapTupleIsValid(tuple))
    3828+
    {
    3829+
    if (is_missing != NULL)
    3830+
    {
    3831+
    /* return "no privileges" instead of throwing an error */
    3832+
    *is_missing = true;
    3833+
    return 0;
    3834+
    }
    3835+
    else
    3836+
    ereport(ERROR,
    3837+
    (errcode(ERRCODE_UNDEFINED_OBJECT),
    3838+
    errmsg("type with OID %u does not exist",
    3839+
    rangetype)));
    3840+
    }
    3841+
    typeForm = (Form_pg_type) GETSTRUCT(tuple);
    3842+
    }
    3843+
    38093844
    /*
    38103845
    * Now get the type's owner and ACL from the tuple
    38113846
    */

    ‎src/backend/catalog/pg_type.c

    Lines changed: 29 additions & 12 deletions
    Original file line numberDiff line numberDiff line change
    @@ -326,14 +326,15 @@ TypeCreate(Oid newTypeOid,
    326326
    errmsg("fixed-size types must have storage PLAIN")));
    327327

    328328
    /*
    329-
    * This is a dependent type if it's an implicitly-created array type, or
    330-
    * if it's a relation rowtype that's not a composite type. For such types
    331-
    * we'll leave the ACL empty, and we'll skip creating some dependency
    332-
    * records because there will be a dependency already through the
    333-
    * depended-on type or relation. (Caution: this is closely intertwined
    334-
    * with some behavior in GenerateTypeDependencies.)
    329+
    * This is a dependent type if it's an implicitly-created array type or
    330+
    * multirange type, or if it's a relation rowtype that's not a composite
    331+
    * type. For such types we'll leave the ACL empty, and we'll skip
    332+
    * creating some dependency records because there will be a dependency
    333+
    * already through the depended-on type or relation. (Caution: this is
    334+
    * closely intertwined with some behavior in GenerateTypeDependencies.)
    335335
    */
    336336
    isDependentType = isImplicitArray ||
    337+
    typeType == TYPTYPE_MULTIRANGE ||
    337338
    (OidIsValid(relationOid) && relationKind != RELKIND_COMPOSITE_TYPE);
    338339

    339340
    /*
    @@ -534,8 +535,9 @@ TypeCreate(Oid newTypeOid,
    534535
    * relationKind and isImplicitArray are likewise somewhat expensive to deduce
    535536
    * from the tuple, so we make callers pass those (they're not optional).
    536537
    *
    537-
    * isDependentType is true if this is an implicit array or relation rowtype;
    538-
    * that means it doesn't need its own dependencies on owner etc.
    538+
    * isDependentType is true if this is an implicit array, multirange, or
    539+
    * relation rowtype; that means it doesn't need its own dependencies on owner
    540+
    * etc.
    539541
    *
    540542
    * We make an extension-membership dependency if we're in an extension
    541543
    * script and makeExtensionDep is true (and isDependentType isn't true).
    @@ -601,18 +603,23 @@ GenerateTypeDependencies(HeapTuple typeTuple,
    601603
    * Make dependencies on namespace, owner, ACL, extension.
    602604
    *
    603605
    * Skip these for a dependent type, since it will have such dependencies
    604-
    * indirectly through its depended-on type or relation.
    606+
    * indirectly through its depended-on type or relation. An exception is
    607+
    * that multiranges need their own namespace dependency, since we don't
    608+
    * force them to be in the same schema as their range type.
    605609
    */
    606610

    607-
    /* placeholder for all normal dependencies */
    611+
    /* collects normal dependencies for bulk recording */
    608612
    addrs_normal = new_object_addresses();
    609613

    610-
    if (!isDependentType)
    614+
    if (!isDependentType || typeForm->typtype == TYPTYPE_MULTIRANGE)
    611615
    {
    612616
    ObjectAddressSet(referenced, NamespaceRelationId,
    613617
    typeForm->typnamespace);
    614-
    recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
    618+
    add_exact_object_address(&referenced, addrs_normal);
    619+
    }
    615620

    621+
    if (!isDependentType)
    622+
    {
    616623
    recordDependencyOnOwner(TypeRelationId, typeObjectId,
    617624
    typeForm->typowner);
    618625

    @@ -727,6 +734,16 @@ GenerateTypeDependencies(HeapTuple typeTuple,
    727734
    recordDependencyOn(&myself, &referenced,
    728735
    isImplicitArray ? DEPENDENCY_INTERNAL : DEPENDENCY_NORMAL);
    729736
    }
    737+
    738+
    /*
    739+
    * Note: you might expect that we should record an internal dependency of
    740+
    * a multirange on its range type here, by analogy with the cases above.
    741+
    * But instead, that is done by RangeCreate(), which also handles
    742+
    * recording of other range-type-specific dependencies. That's pretty
    743+
    * bogus. It's okay for now, because there are no cases where we need to
    744+
    * regenerate the dependencies of a range or multirange type. But someday
    745+
    * we might need to move that logic here to allow such regeneration.
    746+
    */
    730747
    }
    731748

    732749
    /*

    ‎src/backend/commands/typecmds.c

    Lines changed: 36 additions & 6 deletions
    Original file line numberDiff line numberDiff line change
    @@ -3647,6 +3647,8 @@ RenameType(RenameStmt *stmt)
    36473647
    errhint("You can alter type %s, which will alter the array type as well.",
    36483648
    format_type_be(typTup->typelem))));
    36493649

    3650+
    /* we do allow separate renaming of multirange types, though */
    3651+
    36503652
    /*
    36513653
    * If type is composite we need to rename associated pg_class entry too.
    36523654
    * RenameRelationInternal will call RenameTypeInternal automatically.
    @@ -3730,6 +3732,21 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
    37303732
    errhint("You can alter type %s, which will alter the array type as well.",
    37313733
    format_type_be(typTup->typelem))));
    37323734

    3735+
    /* don't allow direct alteration of multirange types, either */
    3736+
    if (typTup->typtype == TYPTYPE_MULTIRANGE)
    3737+
    {
    3738+
    Oid rangetype = get_multirange_range(typeOid);
    3739+
    3740+
    /* We don't expect get_multirange_range to fail, but cope if so */
    3741+
    ereport(ERROR,
    3742+
    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
    3743+
    errmsg("cannot alter multirange type %s",
    3744+
    format_type_be(typeOid)),
    3745+
    OidIsValid(rangetype) ?
    3746+
    errhint("You can alter type %s, which will alter the multirange type as well.",
    3747+
    format_type_be(rangetype)) : 0));
    3748+
    }
    3749+
    37333750
    /*
    37343751
    * If the new owner is the same as the existing owner, consider the
    37353752
    * command to have succeeded. This is for dump restoration purposes.
    @@ -3769,13 +3786,13 @@ AlterTypeOwner(List *names, Oid newOwnerId, ObjectType objecttype)
    37693786
    /*
    37703787
    * AlterTypeOwner_oid - change type owner unconditionally
    37713788
    *
    3772-
    * This function recurses to handle a pg_class entry, if necessary. It
    3773-
    * invokes any necessary access object hooks. If hasDependEntry is true, this
    3774-
    * function modifies the pg_shdepend entry appropriately (this should be
    3775-
    * passed as false only for table rowtypes and array types).
    3789+
    * This function recurses to handle dependent types (arrays and multiranges).
    3790+
    * It invokes any necessary access object hooks. If hasDependEntry is true,
    3791+
    * this function modifies the pg_shdepend entry appropriately (this should be
    3792+
    * passed as false only for table rowtypes and dependent types).
    37763793
    *
    37773794
    * This is used by ALTER TABLE/TYPE OWNER commands, as well as by REASSIGN
    3778-
    * OWNED BY. It assumes the caller has done all needed check.
    3795+
    * OWNED BY. It assumes the caller has done all needed checks.
    37793796
    */
    37803797
    void
    37813798
    AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
    @@ -3815,7 +3832,7 @@ AlterTypeOwner_oid(Oid typeOid, Oid newOwnerId, bool hasDependEntry)
    38153832
    * AlterTypeOwnerInternal - bare-bones type owner change.
    38163833
    *
    38173834
    * This routine simply modifies the owner of a pg_type entry, and recurses
    3818-
    * to handle a possible array type.
    3835+
    * to handle any dependent types.
    38193836
    */
    38203837
    void
    38213838
    AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
    @@ -3865,6 +3882,19 @@ AlterTypeOwnerInternal(Oid typeOid, Oid newOwnerId)
    38653882
    if (OidIsValid(typTup->typarray))
    38663883
    AlterTypeOwnerInternal(typTup->typarray, newOwnerId);
    38673884

    3885+
    /* If it is a range type, update the associated multirange too */
    3886+
    if (typTup->typtype == TYPTYPE_RANGE)
    3887+
    {
    3888+
    Oid multirange_typeid = get_range_multirange(typeOid);
    3889+
    3890+
    if (!OidIsValid(multirange_typeid))
    3891+
    ereport(ERROR,
    3892+
    (errcode(ERRCODE_UNDEFINED_OBJECT),
    3893+
    errmsg("could not find multirange type for data type %s",
    3894+
    format_type_be(typeOid))));
    3895+
    AlterTypeOwnerInternal(multirange_typeid, newOwnerId);
    3896+
    }
    3897+
    38683898
    /* Clean up */
    38693899
    table_close(rel, RowExclusiveLock);
    38703900
    }

    ‎src/bin/pg_dump/pg_dump.c

    Lines changed: 4 additions & 4 deletions
    Original file line numberDiff line numberDiff line change
    @@ -1868,16 +1868,16 @@ selectDumpableType(TypeInfo *tyinfo, Archive *fout)
    18681868
    return;
    18691869
    }
    18701870

    1871-
    /* skip auto-generated array types */
    1871+
    /* skip auto-generated array and multirange types */
    18721872
    if (tyinfo->isArray || tyinfo->isMultirange)
    18731873
    {
    18741874
    tyinfo->dobj.objType = DO_DUMMY_TYPE;
    18751875

    18761876
    /*
    18771877
    * Fall through to set the dump flag; we assume that the subsequent
    18781878
    * rules will do the same thing as they would for the array's base
    1879-
    * type. (We cannot reliably look up the base type here, since
    1880-
    * getTypes may not have processed it yet.)
    1879+
    * type or multirange's range type. (We cannot reliably look up the
    1880+
    * base type here, since getTypes may not have processed it yet.)
    18811881
    */
    18821882
    }
    18831883

    @@ -5770,7 +5770,7 @@ getTypes(Archive *fout, int *numTypes)
    57705770
    else
    57715771
    tyinfo[i].isArray = false;
    57725772

    5773-
    if (tyinfo[i].typtype == 'm')
    5773+
    if (tyinfo[i].typtype == TYPTYPE_MULTIRANGE)
    57745774
    tyinfo[i].isMultirange = true;
    57755775
    else
    57765776
    tyinfo[i].isMultirange = false;

    ‎src/test/regress/expected/dependency.out

    Lines changed: 0 additions & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -144,7 +144,6 @@ owner of sequence deptest_a_seq
    144144
    owner of table deptest
    145145
    owner of function deptest_func()
    146146
    owner of type deptest_enum
    147-
    owner of type deptest_multirange
    148147
    owner of type deptest_range
    149148
    owner of table deptest2
    150149
    owner of sequence ss1

    ‎src/test/regress/expected/multirangetypes.out

    Lines changed: 30 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -3115,6 +3115,36 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
    31153115
    drop type textrange1;
    31163116
    drop type textrange2;
    31173117
    --
    3118+
    -- Multiranges don't have their own ownership or permissions.
    3119+
    --
    3120+
    create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
    3121+
    create role regress_multirange_owner;
    3122+
    alter type multitextrange1 owner to regress_multirange_owner; -- fail
    3123+
    ERROR: cannot alter multirange type multitextrange1
    3124+
    HINT: You can alter type textrange1, which will alter the multirange t EEC0 ype as well.
    3125+
    alter type textrange1 owner to regress_multirange_owner;
    3126+
    set role regress_multirange_owner;
    3127+
    revoke usage on type multitextrange1 from public; -- fail
    3128+
    ERROR: cannot set privileges of multirange types
    3129+
    HINT: Set the privileges of the range type instead.
    3130+
    revoke usage on type textrange1 from public;
    3131+
    \dT+ *textrange1*
    3132+
    List of data types
    3133+
    Schema | Name | Internal name | Size | Elements | Owner | Access privileges | Description
    3134+
    --------+-----------------+-----------------+------+----------+--------------------------+-----------------------------------------------------+-------------
    3135+
    public | multitextrange1 | multitextrange1 | var | | regress_multirange_owner | |
    3136+
    public | textrange1 | textrange1 | var | | regress_multirange_owner | regress_multirange_owner=U/regress_multirange_owner |
    3137+
    (2 rows)
    3138+
    3139+
    create temp table test1(f1 multitextrange1[]);
    3140+
    revoke usage on type textrange1 from regress_multirange_owner;
    3141+
    create temp table test2(f1 multitextrange1[]); -- fail
    3142+
    ERROR: permission denied for type multitextrange1
    3143+
    drop table test1;
    3144+
    drop type textrange1;
    3145+
    reset role;
    3146+
    drop role regress_multirange_owner;
    3147+
    --
    31183148
    -- Test polymorphic type system
    31193149
    --
    31203150
    create function anyarray_anymultirange_func(a anyarray, r anymultirange)

    ‎src/test/regress/sql/multirangetypes.sql

    Lines changed: 21 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -700,6 +700,27 @@ select _textrange1(textrange2('a','z')) @> 'b'::text;
    700700
    drop type textrange1;
    701701
    drop type textrange2;
    702702

    703+
    --
    704+
    -- Multiranges don't have their own ownership or permissions.
    705+
    --
    706+
    create type textrange1 as range(subtype=text, multirange_type_name=multitextrange1, collation="C");
    707+
    create role regress_multirange_owner;
    708+
    709+
    alter type multitextrange1 owner to regress_multirange_owner; -- fail
    710+
    alter type textrange1 owner to regress_multirange_owner;
    711+
    set role regress_multirange_owner;
    712+
    revoke usage on type multitextrange1 from public; -- fail
    713+
    revoke usage on type textrange1 from public;
    714+
    \dT+ *textrange1*
    715+
    create temp table test1(f1 multitextrange1[]);
    716+
    revoke usage on type textrange1 from regress_multirange_owner;
    717+
    create temp table test2(f1 multitextrange1[]); -- fail
    718+
    719+
    drop table test1;
    720+
    drop type textrange1;
    721+
    reset role;
    722+
    drop role regress_multirange_owner;
    723+
    703724
    --
    704725
    -- Test polymorphic type system
    705726
    --

    0 commit comments

    Comments
     (0)
    0