Re: [bug] Table not have typarray when created by single user mode

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

Wenjing

> 2020年7月7日 上午4:22，Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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>&lt;iteration count&gt;</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));
> }
>