Re: pg_upgrade fails to detect unsupported arrays and ranges - Mailing list pgsql-hackers

[ blast-from-the-past department ]

I wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> I can see the appeal of
>> including it before the wrap, even though I personally would've held off.

> Nah, I'm not gonna risk it at this stage.  I concur with your point
> that this is an ancient bug, and one that is unlikely to bite many
> people.  I'll push it Wednesday or so.

I happened across a couple of further pg_upgrade oversights in the
same vein as 29aeda6e4 et al:

* Those commits fixed the bugs in pg_upgrade/version.c about not
checking the contents of arrays/ranges/etc, but there are two
similar functions in pg_upgrade/check.c that I failed to notice
(probably due to the haste with which that patch was prepared).

* We really need to also reject user tables that contain instances
of system-defined composite types (i.e. catalog rowtypes), because
except for a few bootstrap catalogs, those type OIDs are assigned by
genbki.pl not by hand, so they aren't stable across major versions.
For example, in HEAD I get

regression=# select 'pg_enum'::regtype::oid;
  oid  
-------
 13045
(1 row)

but the same OID was 12022 in v13, 11551 in v11, etc.  So if you
had a column of type pg_enum, you'd likely get no-such-type-OID
failures when reading the values after an upgrade.  I don't see
much use-case for doing such a thing, so it seems saner to just
block off the possibility rather than trying to support it.
(We'd have little choice in the back branches anyway, as their
OID values are locked down now.)

The attached proposed patch fixes these cases too.  I generalized
the recursive query a little more so that it could start from an
arbitrary query yielding pg_type OIDs, rather than just one type
name; otherwise it's pretty straightforward.

Barring objections I'll apply and back-patch this soon.

            regards, tom lane

diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index 1c1c908664..ce821a46b3 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -24,6 +24,7 @@ static void check_for_prepared_transactions(ClusterInfo *cluster);
 static void check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster);
 static void check_for_user_defined_postfix_ops(ClusterInfo *cluster);
 static void check_for_tables_with_oids(ClusterInfo *cluster);
+static void check_for_composite_data_type_usage(ClusterInfo *cluster);
 static void check_for_reg_data_type_usage(ClusterInfo *cluster);
 static void check_for_jsonb_9_4_usage(ClusterInfo *cluster);
 static void check_for_pg_role_prefix(ClusterInfo *cluster);
@@ -100,6 +101,7 @@ check_and_dump_old_cluster(bool live_check)
     check_is_install_user(&old_cluster);
     check_proper_datallowconn(&old_cluster);
     check_for_prepared_transactions(&old_cluster);
+    check_for_composite_data_type_usage(&old_cluster);
     check_for_reg_data_type_usage(&old_cluster);
     check_for_isn_and_int8_passing_mismatch(&old_cluster);

@@ -1043,6 +1045,63 @@ check_for_tables_with_oids(ClusterInfo *cluster)
 }


+/*
+ * check_for_composite_data_type_usage()
+ *    Check for system-defined composite types used in user tables.
+ *
+ *    The OIDs of rowtypes of system catalogs and information_schema views
+ *    can change across major versions; unlike user-defined types, we have
+ *    no mechanism for forcing them to be the same in the new cluster.
+ *    Hence, if any user table uses one, that's problematic for pg_upgrade.
+ */
+static void
+check_for_composite_data_type_usage(ClusterInfo *cluster)
+{
+    bool        found;
+    Oid            firstUserOid;
+    char        output_path[MAXPGPATH];
+    char       *base_query;
+
+    prep_status("Checking for system-defined composite types in user tables");
+
+    snprintf(output_path, sizeof(output_path), "tables_using_composite.txt");
+
+    /*
+     * Look for composite types that were made during initdb *or* belong to
+     * information_schema; that's important in case information_schema was
+     * dropped and reloaded.
+     *
+     * The cutoff OID here should match the source cluster's value of
+     * FirstNormalObjectId.  We hardcode it rather than using that C #define
+     * because, if that #define is ever changed, our own version's value is
+     * NOT what to use.  Eventually we may need a test on the source cluster's
+     * version to select the correct value.
+     */
+    firstUserOid = 16384;
+
+    base_query = psprintf("SELECT t.oid FROM pg_catalog.pg_type t "
+                          "LEFT JOIN pg_catalog.pg_namespace n ON t.typnamespace = n.oid "
+                          " WHERE typtype = 'c' AND (t.oid < %u OR nspname = 'information_schema')",
+                          firstUserOid);
+
+    found = check_for_data_types_usage(cluster, base_query, output_path);
+
+    free(base_query);
+
+    if (found)
+    {
+        pg_log(PG_REPORT, "fatal\n");
+        pg_fatal("Your installation contains system-defined composite type(s) in user tables.\n"
+                 "These type OIDs are not stable across PostgreSQL versions,\n"
+                 "so this cluster cannot currently be upgraded.  You can\n"
+                 "remove the problem tables and restart the upgrade.\n"
+                 "A list of the problem columns is in the file:\n"
+                 "    %s\n\n", output_path);
+    }
+    else
+        check_ok();
+}
+
 /*
  * check_for_reg_data_type_usage()
  *    pg_upgrade only preserves these system values:
@@ -1057,88 +1116,29 @@ check_for_tables_with_oids(ClusterInfo *cluster)
 static void
 check_for_reg_data_type_usage(ClusterInfo *cluster)
 {
-    int            dbnum;
-    FILE       *script = NULL;
-    bool        found = false;
+    bool        found;
     char        output_path[MAXPGPATH];

     prep_status("Checking for reg* data types in user tables");

     snprintf(output_path, sizeof(output_path), "tables_using_reg.txt");

-    for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
-    {
-        PGresult   *res;
-        bool        db_used = false;
-        int            ntups;
-        int            rowno;
-        int            i_nspname,
-                    i_relname,
-                    i_attname;
-        DbInfo       *active_db = &cluster->dbarr.dbs[dbnum];
-        PGconn       *conn = connectToServer(cluster, active_db->db_name);
-
-        /*
-         * While several relkinds don't store any data, e.g. views, they can
-         * be used to define data types of other columns, so we check all
-         * relkinds.
-         */
-        res = executeQueryOrDie(conn,
-                                "SELECT n.nspname, c.relname, a.attname "
-                                "FROM    pg_catalog.pg_class c, "
-                                "        pg_catalog.pg_namespace n, "
-                                "        pg_catalog.pg_attribute a, "
-                                "        pg_catalog.pg_type t "
-                                "WHERE    c.oid = a.attrelid AND "
-                                "        NOT a.attisdropped AND "
-                                "       a.atttypid = t.oid AND "
-                                "       t.typnamespace = "
-                                "           (SELECT oid FROM pg_namespace "
-                                "            WHERE nspname = 'pg_catalog') AND"
-                                "        t.typname IN ( "
-        /* regclass.oid is preserved, so 'regclass' is OK */
-                                "           'regcollation', "
-                                "           'regconfig', "
-                                "           'regdictionary', "
-                                "           'regnamespace', "
-                                "           'regoper', "
-                                "           'regoperator', "
-                                "           'regproc', "
-                                "           'regprocedure' "
-        /* regrole.oid is preserved, so 'regrole' is OK */
-        /* regtype.oid is preserved, so 'regtype' is OK */
-                                "            ) AND "
-                                "        c.relnamespace = n.oid AND "
-                                "        n.nspname NOT IN ('pg_catalog', 'information_schema')");
-
-        ntups = PQntuples(res);
-        i_nspname = PQfnumber(res, "nspname");
-        i_relname = PQfnumber(res, "relname");
-        i_attname = PQfnumber(res, "attname");
-        for (rowno = 0; rowno < ntups; rowno++)
-        {
-            found = true;
-            if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
-                         output_path, strerror(errno));
-            if (!db_used)
-            {
-                fprintf(script, "In database: %s\n", active_db->db_name);
-                db_used = true;
-            }
-            fprintf(script, "  %s.%s.%s\n",
-                    PQgetvalue(res, rowno, i_nspname),
-                    PQgetvalue(res, rowno, i_relname),
-                    PQgetvalue(res, rowno, i_attname));
-        }
-
-        PQclear(res);
-
-        PQfinish(conn);
-    }
-
-    if (script)
-        fclose(script);
+    found = check_for_data_types_usage(cluster,
+                                       "SELECT tname::pg_catalog.regtype AS oid"
+                                       " FROM (VALUES "
+    /* pg_class.oid is preserved, so 'regclass' is OK */
+                                       "('pg_catalog.regcollation'), "
+                                       "('pg_catalog.regconfig'), "
+                                       "('pg_catalog.regdictionary'), "
+                                       "('pg_catalog.regnamespace'), "
+                                       "('pg_catalog.regoper'), "
+                                       "('pg_catalog.regoperator'), "
+                                       "('pg_catalog.regproc'), "
+                                       "('pg_catalog.regprocedure') "
+    /* pg_authid.oid is preserved, so 'regrole' is OK */
+    /* pg_type.oid is preserved, so 'regtype' is OK */
+                                       ") v(tname)",
+                                       output_path);

     if (found)
     {
@@ -1163,75 +1163,13 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
 static void
 check_for_jsonb_9_4_usage(ClusterInfo *cluster)
 {
-    int            dbnum;
-    FILE       *script = NULL;
-    bool        found = false;
     char        output_path[MAXPGPATH];

     prep_status("Checking for incompatible \"jsonb\" data type");

     snprintf(output_path, sizeof(output_path), "tables_using_jsonb.txt");

-    for (dbnum = 0; dbnum < cluster->dbarr.ndbs; dbnum++)
-    {
-        PGresult   *res;
-        bool        db_used = false;
-        int            ntups;
-        int            rowno;
-        int            i_nspname,
-                    i_relname,
-                    i_attname;
-        DbInfo       *active_db = &cluster->dbarr.dbs[dbnum];
-        PGconn       *conn = connectToServer(cluster, active_db->db_name);
-
-        /*
-         * While several relkinds don't store any data, e.g. views, they can
-         * be used to define data types of other columns, so we check all
-         * relkinds.
-         */
-        res = executeQueryOrDie(conn,
-                                "SELECT n.nspname, c.relname, a.attname "
-                                "FROM    pg_catalog.pg_class c, "
-                                "        pg_catalog.pg_namespace n, "
-                                "        pg_catalog.pg_attribute a "
-                                "WHERE    c.oid = a.attrelid AND "
-                                "        NOT a.attisdropped AND "
-                                "        a.atttypid = 'pg_catalog.jsonb'::pg_catalog.regtype AND "
-                                "        c.relnamespace = n.oid AND "
-        /* exclude possible orphaned temp tables */
-                                "        n.nspname !~ '^pg_temp_' AND "
-                                "        n.nspname NOT IN ('pg_catalog', 'information_schema')");
-
-        ntups = PQntuples(res);
-        i_nspname = PQfnumber(res, "nspname");
-        i_relname = PQfnumber(res, "relname");
-        i_attname = PQfnumber(res, "attname");
-        for (rowno = 0; rowno < ntups; rowno++)
-        {
-            found = true;
-            if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
-                pg_fatal("could not open file \"%s\": %s\n",
-                         output_path, strerror(errno));
-            if (!db_used)
-            {
-                fprintf(script, "In database: %s\n", active_db->db_name);
-                db_used = true;
-            }
-            fprintf(script, "  %s.%s.%s\n",
-                    PQgetvalue(res, rowno, i_nspname),
-                    PQgetvalue(res, rowno, i_relname),
-                    PQgetvalue(res, rowno, i_attname));
-        }
-
-        PQclear(res);
-
-        PQfinish(conn);
-    }
-
-    if (script)
-        fclose(script);
-
-    if (found)
+    if (check_for_data_type_usage(cluster, "pg_catalog.jsonb", output_path))
     {
         pg_log(PG_REPORT, "fatal\n");
         pg_fatal("Your installation contains the \"jsonb\" data type in user tables.\n"
diff --git a/src/bin/pg_upgrade/pg_upgrade.h b/src/bin/pg_upgrade/pg_upgrade.h
index 919a7849fd..d7666da3f2 100644
--- a/src/bin/pg_upgrade/pg_upgrade.h
+++ b/src/bin/pg_upgrade/pg_upgrade.h
@@ -440,6 +440,12 @@ unsigned int str2uint(const char *str);

 /* version.c */

+bool        check_for_data_types_usage(ClusterInfo *cluster,
+                                       const char *base_query,
+                                       const char *output_path);
+bool        check_for_data_type_usage(ClusterInfo *cluster,
+                                      const char *typename,
+                                      const char *output_path);
 void        new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster,
                                                      bool check_mode);
 void        old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster);
diff --git a/src/bin/pg_upgrade/version.c b/src/bin/pg_upgrade/version.c
index a41247b33d..e7935ce359 100644
--- a/src/bin/pg_upgrade/version.c
+++ b/src/bin/pg_upgrade/version.c
@@ -97,17 +97,22 @@ new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster, bool check_mode)


 /*
- * check_for_data_type_usage
- *    Detect whether there are any stored columns depending on the given type
+ * check_for_data_types_usage()
+ *    Detect whether there are any stored columns depending on given type(s)
  *
  * If so, write a report to the given file name, and return true.
  *
- * We check for the type in tables, matviews, and indexes, but not views;
+ * base_query should be a SELECT yielding a single column named "oid",
+ * containing the pg_type OIDs of one or more types that are known to have
+ * inconsistent on-disk representations across server versions.
+ *
+ * We check for the type(s) in tables, matviews, and indexes, but not views;
  * there's no storage involved in a view.
  */
-static bool
-check_for_data_type_usage(ClusterInfo *cluster, const char *typename,
-                          char *output_path)
+bool
+check_for_data_types_usage(ClusterInfo *cluster,
+                           const char *base_query,
+                           const char *output_path)
 {
     bool        found = false;
     FILE       *script = NULL;
@@ -127,7 +132,7 @@ check_for_data_type_usage(ClusterInfo *cluster, const char *typename,
                     i_attname;

         /*
-         * The type of interest might be wrapped in a domain, array,
+         * The types of interest might be wrapped in a domain, array,
          * composite, or range, and these container types can be nested (to
          * varying extents depending on server version, but that's not of
          * concern here).  To handle all these cases we need a recursive CTE.
@@ -135,8 +140,8 @@ check_for_data_type_usage(ClusterInfo *cluster, const char *typename,
         initPQExpBuffer(&querybuf);
         appendPQExpBuffer(&querybuf,
                           "WITH RECURSIVE oids AS ( "
-        /* the target type itself */
-                          "    SELECT '%s'::pg_catalog.regtype AS oid "
+        /* start with the type(s) returned by base_query */
+                          "    %s "
                           "    UNION ALL "
                           "    SELECT * FROM ( "
         /* inner WITH because we can only reference the CTE once */
@@ -154,7 +159,7 @@ check_for_data_type_usage(ClusterInfo *cluster, const char *typename,
                           "                  c.oid = a.attrelid AND "
                           "                  NOT a.attisdropped AND "
                           "                  a.atttypid = x.oid ",
-                          typename);
+                          base_query);

         /* Ranges were introduced in 9.2 */
         if (GET_MAJOR_VERSION(cluster->major_version) >= 902)
@@ -222,6 +227,34 @@ check_for_data_type_usage(ClusterInfo *cluster, const char *typename,
     return found;
 }

+/*
+ * check_for_data_type_usage()
+ *    Detect whether there are any stored columns depending on the given type
+ *
+ * If so, write a report to the given file name, and return true.
+ *
+ * typename should be a fully qualified type name.  This is just a
+ * trivial wrapper around check_for_data_types_usage() to convert a
+ * type name into a base query.
+ */
+bool
+check_for_data_type_usage(ClusterInfo *cluster,
+                          const char *typename,
+                          const char *output_path)
+{
+    bool        found;
+    char       *base_query;
+
+    base_query = psprintf("SELECT '%s'::pg_catalog.regtype AS oid",
+                          typename);
+
+    found = check_for_data_types_usage(cluster, base_query, output_path);
+
+    free(base_query);
+
+    return found;
+}
+

 /*
  * old_9_3_check_for_line_data_type_usage()