Re: [bug] Table not have typarray when created by single user mode - Mailing list pgsql-bugs

Thank you very much Tom. It looks perfect.
 I don't have any more questions.


Wenjing


> 2020年7月7日 上午4:22，Tom Lane <tgl@sss.pgh.pa.us> 写道：
>
> I wrote:
>> However, if we're going to go this far, I think there's a good
>> case to be made for going all the way and eliminating the policy
>> of not making array types for system catalogs.  That was never
>> anything but a wart justified by space savings in pg_type, and
>> this patch already kills most of the space savings.  If we
>> drop the system-state test in heap_create_with_catalog altogether,
>> we end up with 601 initial pg_type entries.  That still leaves
>> the four bootstrap catalogs without array types, because they are
>> not created by heap_create_with_catalog; but we can manually add
>> those too for a total of 605 initial entries.  (That brings initial
>> pg_type to 14 pages as I show above; I think it was 13 with the
>> original version of the patch.)
>> In short, if we're gonna do this, I think we should do it like
>> the attached.  Or we could do nothing, but there is some appeal
>> to removing this old inconsistency.
>
> I pushed that, but while working on it I had a further thought:
> why is it that we create composite types but not arrays over those
> types for *any* relkinds?  That is, we could create even more
> consistency, as well as buying back some of the pg_type bloat added
> here, by not creating pg_type entries at all for toast tables or
> sequences.  A little bit of hacking later, I have the attached.
>
> One could argue it either way as to whether sequences should have
> composite types.  It's possible to demonstrate queries that will
> fail without one:
>
> regression=# create sequence seq1;
> CREATE SEQUENCE
> regression=# select s from seq1 s;
> ERROR:  relation "seq1" does not have a composite type
>
> but it's pretty hard to believe anyone's using that in practice.
> Also, we've talked more than once about changing the implementation
> of sequences to not have a relation per sequence, in which case the
> ability to do something like the above would go away anyway.
>
> Comments?
>
>             regards, tom lane
>
> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index 003d278370..7471ba53f2 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -1895,7 +1895,8 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>       </para>
>       <para>
>        The OID of the data type that corresponds to this table's row type,
> -       if any (zero for indexes, which have no <structname>pg_type</structname> entry)
> +       if any (zero for indexes, sequences, and toast tables, which have
> +       no <structname>pg_type</structname> entry)
>       </para></entry>
>      </row>
>
> diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
> index fd04e82b20..ae509d9d49 100644
> --- a/src/backend/catalog/heap.c
> +++ b/src/backend/catalog/heap.c
> @@ -1118,8 +1118,6 @@ heap_create_with_catalog(const char *relname,
>     Oid            existing_relid;
>     Oid            old_type_oid;
>     Oid            new_type_oid;
> -    ObjectAddress new_type_addr;
> -    Oid            new_array_oid = InvalidOid;
>     TransactionId relfrozenxid;
>     MultiXactId relminmxid;
>
> @@ -1262,44 +1260,45 @@ heap_create_with_catalog(const char *relname,
>     new_rel_desc->rd_rel->relrewrite = relrewrite;
>
>     /*
> -     * Decide whether to create an array type over the relation's rowtype.
> -     * Array types are made except where the use of a relation as such is an
> +     * Decide whether to create a pg_type entry for the relation's rowtype.
> +     * These types are made except where the use of a relation as such is an
>      * implementation detail: toast tables, sequences and indexes.
>      */
>     if (!(relkind == RELKIND_SEQUENCE ||
>           relkind == RELKIND_TOASTVALUE ||
>           relkind == RELKIND_INDEX ||
>           relkind == RELKIND_PARTITIONED_INDEX))
> -        new_array_oid = AssignTypeArrayOid();
> -
> -    /*
> -     * Since defining a relation also defines a complex type, we add a new
> -     * system type corresponding to the new relation.  The OID of the type can
> -     * be preselected by the caller, but if reltypeid is InvalidOid, we'll
> -     * generate a new OID for it.
> -     *
> -     * NOTE: we could get a unique-index failure here, in case someone else is
> -     * creating the same type name in parallel but hadn't committed yet when
> -     * we checked for a duplicate name above.
> -     */
> -    new_type_addr = AddNewRelationType(relname,
> -                                       relnamespace,
> -                                       relid,
> -                                       relkind,
> -                                       ownerid,
> -                                       reltypeid,
> -                                       new_array_oid);
> -    new_type_oid = new_type_addr.objectId;
> -    if (typaddress)
> -        *typaddress = new_type_addr;
> -
> -    /*
> -     * Now make the array type if wanted.
> -     */
> -    if (OidIsValid(new_array_oid))
>     {
> +        Oid            new_array_oid;
> +        ObjectAddress new_type_addr;
>         char       *relarrayname;
>
> +        /*
> +         * We'll make an array over the composite type, too.  For largely
> +         * historical reasons, the array type's OID is assigned first.
> +         */
> +        new_array_oid = AssignTypeArrayOid();
> +
> +        /*
> +         * The OID of the composite type can be preselected by the caller, but
> +         * if reltypeid is InvalidOid, we'll generate a new OID for it.
> +         *
> +         * NOTE: we could get a unique-index failure here, in case someone
> +         * else is creating the same type name in parallel but hadn't
> +         * committed yet when we checked for a duplicate name above.
> +         */
> +        new_type_addr = AddNewRelationType(relname,
> +                                           relnamespace,
> +                                           relid,
> +                                           relkind,
> +                                           ownerid,
> +                                           reltypeid,
> +                                           new_array_oid);
> +        new_type_oid = new_type_addr.objectId;
> +        if (typaddress)
> +            *typaddress = new_type_addr;
> +
> +        /* Now create the array type. */
>         relarrayname = makeArrayTypeName(relname, relnamespace);
>
>         TypeCreate(new_array_oid,    /* force the type's OID to this */
> @@ -1336,6 +1335,14 @@ heap_create_with_catalog(const char *relname,
>
>         pfree(relarrayname);
>     }
> +    else
> +    {
> +        /* Caller should not be expecting a type to be created. */
> +        Assert(reltypeid == InvalidOid);
> +        Assert(typaddress == NULL);
> +
> +        new_type_oid = InvalidOid;
> +    }
>
>     /*
>      * now create an entry in pg_class for the relation.
> diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
> index 3f7ab8d389..8b8888af5e 100644
> --- a/src/backend/catalog/toasting.c
> +++ b/src/backend/catalog/toasting.c
> @@ -34,9 +34,6 @@
> #include "utils/rel.h"
> #include "utils/syscache.h"
>
> -/* Potentially set by pg_upgrade_support functions */
> -Oid            binary_upgrade_next_toast_pg_type_oid = InvalidOid;
> -
> static void CheckAndCreateToastTable(Oid relOid, Datum reloptions,
>                                      LOCKMODE lockmode, bool check);
> static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
> @@ -135,7 +132,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
>     Relation    toast_rel;
>     Relation    class_rel;
>     Oid            toast_relid;
> -    Oid            toast_typid = InvalidOid;
>     Oid            namespaceid;
>     char        toast_relname[NAMEDATALEN];
>     char        toast_idxname[NAMEDATALEN];
> @@ -181,8 +177,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
>          * problem that it might take up an OID that will conflict with some
>          * old-cluster table we haven't seen yet.
>          */
> -        if (!OidIsValid(binary_upgrade_next_toast_pg_class_oid) ||
> -            !OidIsValid(binary_upgrade_next_toast_pg_type_oid))
> +        if (!OidIsValid(binary_upgrade_next_toast_pg_class_oid))
>             return false;
>     }
>
> @@ -234,17 +229,6 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
>     else
>         namespaceid = PG_TOAST_NAMESPACE;
>
> -    /*
> -     * Use binary-upgrade override for pg_type.oid, if supplied.  We might be
> -     * in the post-schema-restore phase where we are doing ALTER TABLE to
> -     * create TOAST tables that didn't exist in the old cluster.
> -     */
> -    if (IsBinaryUpgrade && OidIsValid(binary_upgrade_next_toast_pg_type_oid))
> -    {
> -        toast_typid = binary_upgrade_next_toast_pg_type_oid;
> -        binary_upgrade_next_toast_pg_type_oid = InvalidOid;
> -    }
> -
>     /* Toast table is shared if and only if its parent is. */
>     shared_relation = rel->rd_rel->relisshared;
>
> @@ -255,7 +239,7 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
>                                            namespaceid,
>                                            rel->rd_rel->reltablespace,
>                                            toastOid,
> -                                           toast_typid,
> +                                           InvalidOid,
>                                            InvalidOid,
>                                            rel->rd_rel->relowner,
>                                            table_relation_toast_am(rel),
> diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
> index f79044f39f..4b2548f33f 100644
> --- a/src/backend/commands/tablecmds.c
> +++ b/src/backend/commands/tablecmds.c
> @@ -12564,8 +12564,7 @@ ATExecChangeOwner(Oid relationOid, Oid newOwnerId, bool recursing, LOCKMODE lock
>         /*
>          * Also change the ownership of the table's row type, if it has one
>          */
> -        if (tuple_class->relkind != RELKIND_INDEX &&
> -            tuple_class->relkind != RELKIND_PARTITIONED_INDEX)
> +        if (OidIsValid(tuple_class->reltype))
>             AlterTypeOwnerInternal(tuple_class->reltype, newOwnerId);
>
>         /*
> @@ -15009,9 +15008,10 @@ AlterTableNamespaceInternal(Relation rel, Oid oldNspOid, Oid nspOid,
>     AlterRelationNamespaceInternal(classRel, RelationGetRelid(rel), oldNspOid,
>                                    nspOid, true, objsMoved);
>
> -    /* Fix the table's row type too */
> -    AlterTypeNamespaceInternal(rel->rd_rel->reltype,
> -                               nspOid, false, false, objsMoved);
> +    /* Fix the table's row type too, if it has one */
> +    if (OidIsValid(rel->rd_rel->reltype))
> +        AlterTypeNamespaceInternal(rel->rd_rel->reltype,
> +                                   nspOid, false, false, objsMoved);
>
>     /* Fix other dependent stuff */
>     if (rel->rd_rel->relkind == RELKIND_RELATION ||
> @@ -15206,11 +15206,11 @@ AlterSeqNamespaces(Relation classRel, Relation rel,
>                                        true, objsMoved);
>
>         /*
> -         * Sequences have entries in pg_type. We need to be careful to move
> -         * them to the new namespace, too.
> +         * Sequences used to have entries in pg_type, but no longer do.  If we
> +         * ever re-instate that, we'll need to move the pg_type entry to the
> +         * new namespace, too (using AlterTypeNamespaceInternal).
>          */
> -        AlterTypeNamespaceInternal(RelationGetForm(seqRel)->reltype,
> -                                   newNspOid, false, false, objsMoved);
> +        Assert(RelationGetForm(seqRel)->reltype == InvalidOid);
>
>         /* Now we can close it.  Keep the lock till end of transaction. */
>         relation_close(seqRel, NoLock);
> diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
> index b442b5a29e..49de285f01 100644
> --- a/src/backend/nodes/makefuncs.c
> +++ b/src/backend/nodes/makefuncs.c
> @@ -145,8 +145,10 @@ makeWholeRowVar(RangeTblEntry *rte,
>             /* relation: the rowtype is a named composite type */
>             toid = get_rel_type_id(rte->relid);
>             if (!OidIsValid(toid))
> -                elog(ERROR, "could not find type OID for relation %u",
> -                     rte->relid);
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                         errmsg("relation \"%s\" does not have a composite type",
> +                                get_rel_name(rte->relid))));
>             result = makeVar(varno,
>                              InvalidAttrNumber,
>                              toid,
> diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c
> index 18f2ee8226..14d9eb2b5b 100644
> --- a/src/backend/utils/adt/pg_upgrade_support.c
> +++ b/src/backend/utils/adt/pg_upgrade_support.c
> @@ -51,17 +51,6 @@ binary_upgrade_set_next_array_pg_type_oid(PG_FUNCTION_ARGS)
>     PG_RETURN_VOID();
> }
>
> -Datum
> -binary_upgrade_set_next_toast_pg_type_oid(PG_FUNCTION_ARGS)
> -{
> -    Oid            typoid = PG_GETARG_OID(0);
> -
> -    CHECK_IS_BINARY_UPGRADE;
> -    binary_upgrade_next_toast_pg_type_oid = typoid;
> -
> -    PG_RETURN_VOID();
> -}
> -
> Datum
> binary_upgrade_set_next_heap_pg_class_oid(PG_FUNCTION_ARGS)
> {
> diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
> index a41a3db876..ee0947dda7 100644
> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -272,7 +272,7 @@ static void binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
>                                                      PQExpBuffer upgrade_buffer,
>                                                      Oid pg_type_oid,
>                                                      bool force_array_type);
> -static bool binary_upgrade_set_type_oids_by_rel_oid(Archive *fout,
> +static void binary_upgrade_set_type_oids_by_rel_oid(Archive *fout,
>                                                     PQExpBuffer upgrade_buffer, Oid pg_rel_oid);
> static void binary_upgrade_set_pg_class_oids(Archive *fout,
>                                              PQExpBuffer upgrade_buffer,
> @@ -4493,7 +4493,7 @@ binary_upgrade_set_type_oids_by_type_oid(Archive *fout,
>     destroyPQExpBuffer(upgrade_query);
> }
>
> -static bool
> +static void
> binary_upgrade_set_type_oids_by_rel_oid(Archive *fout,
>                                         PQExpBuffer upgrade_buffer,
>                                         Oid pg_rel_oid)
> @@ -4501,48 +4501,23 @@ binary_upgrade_set_type_oids_by_rel_oid(Archive *fout,
>     PQExpBuffer upgrade_query = createPQExpBuffer();
>     PGresult   *upgrade_res;
>     Oid            pg_type_oid;
> -    bool        toast_set = false;
>
> -    /*
> -     * We only support old >= 8.3 for binary upgrades.
> -     *
> -     * We purposefully ignore toast OIDs for partitioned tables; the reason is
> -     * that versions 10 and 11 have them, but 12 does not, so emitting them
> -     * causes the upgrade to fail.
> -     */
>     appendPQExpBuffer(upgrade_query,
> -                      "SELECT c.reltype AS crel, t.reltype AS trel "
> +                      "SELECT c.reltype AS crel "
>                       "FROM pg_catalog.pg_class c "
> -                      "LEFT JOIN pg_catalog.pg_class t ON "
> -                      "  (c.reltoastrelid = t.oid AND c.relkind <> '%c') "
>                       "WHERE c.oid = '%u'::pg_catalog.oid;",
> -                      RELKIND_PARTITIONED_TABLE, pg_rel_oid);
> +                      pg_rel_oid);
>
>     upgrade_res = ExecuteSqlQueryForSingleRow(fout, upgrade_query->data);
>
>     pg_type_oid = atooid(PQgetvalue(upgrade_res, 0, PQfnumber(upgrade_res, "crel")));
>
> -    binary_upgrade_set_type_oids_by_type_oid(fout, upgrade_buffer,
> -                                             pg_type_oid, false);
> -
> -    if (!PQgetisnull(upgrade_res, 0, PQfnumber(upgrade_res, "trel")))
> -    {
> -        /* Toast tables do not have pg_type array rows */
> -        Oid            pg_type_toast_oid = atooid(PQgetvalue(upgrade_res, 0,
> -                                                          PQfnumber(upgrade_res, "trel")));
> -
> -        appendPQExpBufferStr(upgrade_buffer, "\n-- For binary upgrade, must preserve pg_type toast oid\n");
> -        appendPQExpBuffer(upgrade_buffer,
> -                          "SELECT pg_catalog.binary_upgrade_set_next_toast_pg_type_oid('%u'::pg_catalog.oid);\n\n",
> -                          pg_type_toast_oid);
> -
> -        toast_set = true;
> -    }
> +    if (OidIsValid(pg_type_oid))
> +        binary_upgrade_set_type_oids_by_type_oid(fout, upgrade_buffer,
> +                                                 pg_type_oid, false);
>
>     PQclear(upgrade_res);
>     destroyPQExpBuffer(upgrade_query);
> -
> -    return toast_set;
> }
>
> static void
> diff --git a/src/include/catalog/binary_upgrade.h b/src/include/catalog/binary_upgrade.h
> index 12d94fe1b3..02fecb90f7 100644
> --- a/src/include/catalog/binary_upgrade.h
> +++ b/src/include/catalog/binary_upgrade.h
> @@ -16,7 +16,6 @@
>
> extern PGDLLIMPORT Oid binary_upgrade_next_pg_type_oid;
> extern PGDLLIMPORT Oid binary_upgrade_next_array_pg_type_oid;
> -extern PGDLLIMPORT Oid binary_upgrade_next_toast_pg_type_oid;
>
> extern PGDLLIMPORT Oid binary_upgrade_next_heap_pg_class_oid;
> extern PGDLLIMPORT Oid binary_upgrade_next_index_pg_class_oid;
> diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
> index 38295aca48..a995a104b6 100644
> --- a/src/include/catalog/pg_proc.dat
> +++ b/src/include/catalog/pg_proc.dat
> @@ -10306,10 +10306,6 @@
>   proname => 'binary_upgrade_set_next_array_pg_type_oid', provolatile => 'v',
>   proparallel => 'r', prorettype => 'void', proargtypes => 'oid',
>   prosrc => 'binary_upgrade_set_next_array_pg_type_oid' },
> -{ oid => '3585', descr => 'for use by pg_upgrade',
> -  proname => 'binary_upgrade_set_next_toast_pg_type_oid', provolatile => 'v',
> -  proparallel => 'r', prorettype => 'void', proargtypes => 'oid',
> -  prosrc => 'binary_upgrade_set_next_toast_pg_type_oid' },
> { oid => '3586', descr => 'for use by pg_upgrade',
>   proname => 'binary_upgrade_set_next_heap_pg_class_oid', provolatile => 'v',
>   proparallel => 'r', prorettype => 'void', proargtypes => 'oid',
> diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
> index 828ff5a288..e7f4a5f291 100644
> --- a/src/pl/plpgsql/src/pl_comp.c
> +++ b/src/pl/plpgsql/src/pl_comp.c
> @@ -1778,6 +1778,7 @@ PLpgSQL_type *
> plpgsql_parse_wordrowtype(char *ident)
> {
>     Oid            classOid;
> +    Oid            typOid;
>
>     /*
>      * Look up the relation.  Note that because relation rowtypes have the
> @@ -1792,8 +1793,16 @@ plpgsql_parse_wordrowtype(char *ident)
>                 (errcode(ERRCODE_UNDEFINED_TABLE),
>                  errmsg("relation \"%s\" does not exist", ident)));
>
> +    /* Some relkinds lack type OIDs */
> +    typOid = get_rel_type_id(classOid);
> +    if (!OidIsValid(typOid))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                 errmsg("relation \"%s\" does not have a composite type",
> +                        ident)));
> +
>     /* Build and return the row type struct */
> -    return plpgsql_build_datatype(get_rel_type_id(classOid), -1, InvalidOid,
> +    return plpgsql_build_datatype(typOid, -1, InvalidOid,
>                                   makeTypeName(ident));
> }
>
> @@ -1806,6 +1815,7 @@ PLpgSQL_type *
> plpgsql_parse_cwordrowtype(List *idents)
> {
>     Oid            classOid;
> +    Oid            typOid;
>     RangeVar   *relvar;
>     MemoryContext oldCxt;
>
> @@ -1825,10 +1835,18 @@ plpgsql_parse_cwordrowtype(List *idents)
>                           -1);
>     classOid = RangeVarGetRelid(relvar, NoLock, false);
>
> +    /* Some relkinds lack type OIDs */
> +    typOid = get_rel_type_id(classOid);
> +    if (!OidIsValid(typOid))
> +        ereport(ERROR,
> +                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +                 errmsg("relation \"%s\" does not have a composite type",
> +                        strVal(lsecond(idents)))));
> +
>     MemoryContextSwitchTo(oldCxt);
>
>     /* Build and return the row type struct */
> -    return plpgsql_build_datatype(get_rel_type_id(classOid), -1, InvalidOid,
> +    return plpgsql_build_datatype(typOid, -1, InvalidOid,
>                                   makeTypeNameFromNameList(idents));
> }
>