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

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));
 }