Thread: Refactoring pg_dump's getTables()

Refactoring pg_dump's getTables()

From
Tom Lane
Date:
Attached is a proposed patch that refactors getTables() along the
same lines as some previous work (eg 047329624, ed2c7f65b, daa9fe8a5)
to avoid having multiple partially-redundant copies of the SQL query.
This gets rid of nearly 300 lines of duplicative spaghetti code,
creates a uniform style for dealing with cross-version changes
(replacing at least three different methods currently being used
for that in this same stretch of code), and allows moving some
comments to be closer to the code they describe.

There's a lot I still want to change here, but this part seems like it
should be fairly uncontroversial.  I've tested it against servers back
to 8.0 (which is what led me to trip over the bug fixed in 40dfac4fc).
Any objections to just pushing it?

            regards, tom lane

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 6ec524f8e6..e35ff6e7fb 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6274,65 +6274,152 @@ getTables(Archive *fout, int *numTables)
      * We include system catalogs, so that we can work if a user table is
      * defined to inherit from a system catalog (pretty weird, but...)
      *
-     * We ignore relations that are not ordinary tables, sequences, views,
-     * materialized views, composite types, or foreign tables.
-     *
-     * Composite-type table entries won't be dumped as such, but we have to
-     * make a DumpableObject for them so that we can track dependencies of the
-     * composite type (pg_depend entries for columns of the composite type
-     * link to the pg_class entry not the pg_type entry).
-     *
      * Note: in this phase we should collect only a minimal amount of
      * information about each table, basically just enough to decide if it is
      * interesting. We must fetch all tables in this phase because otherwise
      * we cannot correctly identify inherited columns, owned sequences, etc.
-     *
-     * 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(query,
+                      "SELECT c.tableoid, c.oid, c.relname, "
+                      "c.relnamespace, c.relkind, "
+                      "(%s c.relowner) AS rolname, "
+                      "c.relchecks, "
+                      "c.relhasindex, c.relhasrules, c.relpages, "
+                      "d.refobjid AS owning_tab, "
+                      "d.refobjsubid AS owning_col, "
+                      "tsp.spcname AS reltablespace, ",
+                      username_subquery);
+
+    if (fout->remoteVersion >= 80400)
+        appendPQExpBufferStr(query,
+                             "c.relhastriggers, ");
+    else
+        appendPQExpBufferStr(query,
+                             "(c.reltriggers <> 0) AS relhastriggers, ");
+
+    if (fout->remoteVersion >= 90500)
+        appendPQExpBufferStr(query,
+                             "c.relrowsecurity, c.relforcerowsecurity, ");
+    else
+        appendPQExpBufferStr(query,
+                             "false AS relrowsecurity, "
+                             "false AS relforcerowsecurity, ");
+
+    if (fout->remoteVersion >= 120000)
+        appendPQExpBufferStr(query,
+                             "false AS relhasoids, ");
+    else
+        appendPQExpBufferStr(query,
+                             "c.relhasoids, ");
+
+    if (fout->remoteVersion >= 80200)
+        appendPQExpBufferStr(query,
+                             "c.relfrozenxid, tc.relfrozenxid AS tfrozenxid, ");
+    else
+        appendPQExpBufferStr(query,
+                             "0 AS relfrozenxid, 0 AS tfrozenxid, ");
+
+    if (fout->remoteVersion >= 90300)
+        appendPQExpBufferStr(query,
+                             "c.relminmxid, tc.relminmxid AS tminmxid, ");
+    else
+        appendPQExpBufferStr(query,
+                             "0 AS relminmxid, 0 AS tminmxid, ");
+
+    if (fout->remoteVersion >= 80200)
+        appendPQExpBufferStr(query,
+                             "tc.oid AS toid, ");
+    else
+        appendPQExpBufferStr(query,
+                             "0 AS toid, ");
+
+    if (fout->remoteVersion >= 90100)
+        appendPQExpBufferStr(query,
+                             "c.relpersistence, ");
+    else
+        appendPQExpBufferStr(query,
+                             "'p' AS relpersistence, ");
+
+    if (fout->remoteVersion >= 90300)
+        appendPQExpBufferStr(query,
+                             "c.relispopulated, ");
+    else
+        appendPQExpBufferStr(query,
+                             "'t' as relispopulated, ");
+
+    if (fout->remoteVersion >= 90400)
+        appendPQExpBufferStr(query,
+                             "c.relreplident, ");
+    else
+        appendPQExpBufferStr(query,
+                             "'d' AS relreplident, ");
+
+    if (fout->remoteVersion >= 90100)
+        appendPQExpBufferStr(query,
+                             "CASE WHEN c.relkind = " CppAsString2(RELKIND_FOREIGN_TABLE) " THEN "
+                             "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
+                             "ELSE 0 END AS foreignserver, ");
+    else
+        appendPQExpBufferStr(query,
+                             "0 AS foreignserver, ");
+
+    if (fout->remoteVersion >= 90300)
+        appendPQExpBufferStr(query,
+                             "array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded') AS
reloptions," 
+                             "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
+                             "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS
checkoption,"); 
+    else if (fout->remoteVersion >= 80200)
+        appendPQExpBufferStr(query,
+                             "c.reloptions, NULL AS checkoption, ");
+    else
+        appendPQExpBufferStr(query,
+                             "NULL AS reloptions, NULL AS checkoption, ");
+
+    if (fout->remoteVersion >= 80400)
+        appendPQExpBufferStr(query,
+                             "tc.reloptions AS toast_reloptions, ");
+    else
+        appendPQExpBufferStr(query,
+                             "NULL AS toast_reloptions, ");
+
+    if (fout->remoteVersion >= 90000)
+        appendPQExpBufferStr(query,
+                             "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS
reloftype,"); 
+    else
+        appendPQExpBufferStr(query,
+                             "NULL AS reloftype, ");
+
     if (fout->remoteVersion >= 90600)
-    {
-        char       *partkeydef = "NULL";
-        char       *ispartition = "false";
-        char       *partbound = "NULL";
-        char       *relhasoids = "c.relhasoids";
+        appendPQExpBufferStr(query,
+                             "c.relkind = " CppAsString2(RELKIND_SEQUENCE)
+                             " AND EXISTS (SELECT 1 FROM pg_depend "
+                             "WHERE classid = 'pg_class'::regclass AND "
+                             "objid = c.oid AND objsubid = 0 AND "
+                             "refclassid = 'pg_class'::regclass AND "
+                             "deptype = 'i') AS is_identity_sequence, ");
+    else
+        appendPQExpBufferStr(query,
+                             "false AS is_identity_sequence, ");

+    if (fout->remoteVersion >= 90600)
+        appendPQExpBufferStr(query,
+                             "am.amname, ");
+    else
+        appendPQExpBufferStr(query,
+                             "NULL AS amname, ");
+
+    if (fout->remoteVersion >= 90600)
+    {
         PQExpBuffer acl_subquery = createPQExpBuffer();
         PQExpBuffer racl_subquery = createPQExpBuffer();
         PQExpBuffer initacl_subquery = createPQExpBuffer();
         PQExpBuffer initracl_subquery = createPQExpBuffer();
-
         PQExpBuffer attacl_subquery = createPQExpBuffer();
         PQExpBuffer attracl_subquery = createPQExpBuffer();
         PQExpBuffer attinitacl_subquery = createPQExpBuffer();
         PQExpBuffer attinitracl_subquery = createPQExpBuffer();

-        /*
-         * Collect the information about any partitioned tables, which were
-         * added in PG10.
-         */
-
-        if (fout->remoteVersion >= 100000)
-        {
-            partkeydef = "pg_get_partkeydef(c.oid)";
-            ispartition = "c.relispartition";
-            partbound = "pg_get_expr(c.relpartbound, c.oid)";
-        }
-
-        /* In PG12 upwards WITH OIDS does not exist anymore. */
-        if (fout->remoteVersion >= 120000)
-            relhasoids = "'f'::bool";
-
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         *
-         * Left join to detect if any privileges are still as-set-at-init, in
-         * which case we won't dump out ACL commands for those.
-         */
-
         buildACLQueries(acl_subquery, racl_subquery, initacl_subquery,
                         initracl_subquery, "c.relacl", "c.relowner",
                         "pip.initprivs",
@@ -6345,31 +6432,14 @@ getTables(Archive *fout, int *numTables)
                         "pip.initprivs", "'c'", dopt->binary_upgrade);

         appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
                           "%s AS relacl, %s as rrelacl, "
-                          "%s AS initrelacl, %s as initrrelacl, "
-                          "c.relkind, c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, %s AS relhasoids, "
-                          "c.relrowsecurity, c.relforcerowsecurity, "
-                          "c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "tc.relminmxid AS tminmxid, "
-                          "c.relpersistence, c.relispopulated, "
-                          "c.relreplident, c.relpages, am.amname, "
-                          "CASE WHEN c.relkind = 'f' THEN "
-                          "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
-                          "ELSE 0 END AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded') AS
reloptions," 
-                          "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
-                          "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS
checkoption," 
-                          "tc.reloptions AS toast_reloptions, "
-                          "c.relkind = '%c' AND EXISTS (SELECT 1 FROM pg_depend WHERE classid = 'pg_class'::regclass
ANDobjid = c.oid AND objsubid = 0 AND refclassid = 'pg_class'::regclass AND deptype = 'i') AS is_identity_sequence, " 
+                          "%s AS initrelacl, %s as initrrelacl, ",
+                          acl_subquery->data,
+                          racl_subquery->data,
+                          initacl_subquery->data,
+                          initracl_subquery->data);
+
+        appendPQExpBuffer(query,
                           "EXISTS (SELECT 1 FROM pg_attribute at LEFT JOIN pg_init_privs pip ON "
                           "(c.oid = pip.objoid "
                           "AND pip.classoid = 'pg_class'::regclass "
@@ -6380,454 +6450,96 @@ getTables(Archive *fout, int *numTables)
                           "OR %s IS NOT NULL "
                           "OR %s IS NOT NULL"
                           "))"
-                          "AS changed_acl, "
-                          "%s AS partkeydef, "
-                          "%s AS ispartition, "
-                          "%s AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype IN ('a', 'i')) "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid AND c.relkind <> '%c') "
-                          "LEFT JOIN pg_am am ON (c.relam = am.oid) "
-                          "LEFT JOIN pg_init_privs pip ON "
-                          "(c.oid = pip.objoid "
-                          "AND pip.classoid = 'pg_class'::regclass "
-                          "AND pip.objsubid = 0) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          acl_subquery->data,
-                          racl_subquery->data,
-                          initacl_subquery->data,
-                          initracl_subquery->data,
-                          username_subquery,
-                          relhasoids,
-                          RELKIND_SEQUENCE,
+                          "AS changed_acl, ",
                           attacl_subquery->data,
                           attracl_subquery->data,
                           attinitacl_subquery->data,
-                          attinitracl_subquery->data,
-                          partkeydef,
-                          ispartition,
-                          partbound,
-                          RELKIND_SEQUENCE,
-                          RELKIND_PARTITIONED_TABLE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
-                          RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE,
-                          RELKIND_PARTITIONED_TABLE);
+                          attinitracl_subquery->data);

         destroyPQExpBuffer(acl_subquery);
         destroyPQExpBuffer(racl_subquery);
         destroyPQExpBuffer(initacl_subquery);
         destroyPQExpBuffer(initracl_subquery);
-
         destroyPQExpBuffer(attacl_subquery);
         destroyPQExpBuffer(attracl_subquery);
         destroyPQExpBuffer(attinitacl_subquery);
         destroyPQExpBuffer(attinitracl_subquery);
     }
-    else if (fout->remoteVersion >= 90500)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "c.relrowsecurity, c.relforcerowsecurity, "
-                          "c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "tc.relminmxid AS tminmxid, "
-                          "c.relpersistence, c.relispopulated, "
-                          "c.relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "CASE WHEN c.relkind = 'f' THEN "
-                          "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
-                          "ELSE 0 END AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded') AS
reloptions," 
-                          "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
-                          "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS
checkoption," 
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
-                          RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE);
-    }
-    else if (fout->remoteVersion >= 90400)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "tc.relminmxid AS tminmxid, "
-                          "c.relpersistence, c.relispopulated, "
-                          "c.relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "CASE WHEN c.relkind = 'f' THEN "
-                          "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
-                          "ELSE 0 END AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded') AS
reloptions," 
-                          "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
-                          "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS
checkoption," 
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
-                          RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE);
-    }
-    else if (fout->remoteVersion >= 90300)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, c.relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "tc.relminmxid AS tminmxid, "
-                          "c.relpersistence, c.relispopulated, "
-                          "'d' AS relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "CASE WHEN c.relkind = 'f' THEN "
-                          "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
-                          "ELSE 0 END AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded') AS
reloptions," 
-                          "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
-                          "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL END AS
checkoption," 
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
-                          RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE);
-    }
-    else if (fout->remoteVersion >= 90100)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "0 AS tminmxid, "
-                          "c.relpersistence, 't' as relispopulated, "
-                          "'d' AS relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "CASE WHEN c.relkind = 'f' THEN "
-                          "(SELECT ftserver FROM pg_catalog.pg_foreign_table WHERE ftrelid = c.oid) "
-                          "ELSE 0 END AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "c.reloptions AS reloptions, "
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE,
-                          RELKIND_MATVIEW, RELKIND_FOREIGN_TABLE);
-    }
-    else if (fout->remoteVersion >= 90000)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "0 AS tminmxid, "
-                          "'p' AS relpersistence, 't' as relispopulated, "
-                          "'d' AS relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "NULL AS foreignserver, "
-                          "CASE WHEN c.reloftype <> 0 THEN c.reloftype::pg_catalog.regtype ELSE NULL END AS reloftype,
"
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "c.reloptions AS reloptions, "
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE);
-    }
-    else if (fout->remoteVersion >= 80400)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, c.relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "0 AS tminmxid, "
-                          "'p' AS relpersistence, 't' as relispopulated, "
-                          "'d' AS relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "NULL AS foreignserver, "
-                          "NULL AS reloftype, "
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "c.reloptions AS reloptions, "
-                          "tc.reloptions AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE);
-    }
-    else if (fout->remoteVersion >= 80200)
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any (note this dependency is AUTO as of 8.2)
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, c.relname, "
-                          "c.relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "c.relkind, "
-                          "c.relnamespace, "
-                          "(%s c.relowner) AS rolname, "
-                          "c.relchecks, (c.reltriggers <> 0) AS relhastriggers, "
-                          "c.relhasindex, c.relhasrules, c.relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "c.relfrozenxid, 0 AS relminmxid, tc.oid AS toid, "
-                          "tc.relfrozenxid AS tfrozenxid, "
-                          "0 AS tminmxid, "
-                          "'p' AS relpersistence, 't' as relispopulated, "
-                          "'d' AS relreplident, c.relpages, "
-                          "NULL AS amname, "
-                          "NULL AS foreignserver, "
-                          "NULL AS reloftype, "
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "c.reloptions AS reloptions, "
-                          "NULL AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'a') "
-                          "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid) "
-                          "WHERE c.relkind in ('%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE);
-    }
     else
-    {
-        /*
-         * Left join to pick up dependency info linking sequences to their
-         * owning column, if any
-         */
-        appendPQExpBuffer(query,
-                          "SELECT c.tableoid, c.oid, relname, "
-                          "relacl, NULL as rrelacl, "
-                          "NULL AS initrelacl, NULL AS initrrelacl, "
-                          "relkind, relnamespace, "
-                          "(%s relowner) AS rolname, "
-                          "relchecks, (reltriggers <> 0) AS relhastriggers, "
-                          "relhasindex, relhasrules, relhasoids, "
-                          "'f'::bool AS relrowsecurity, "
-                          "'f'::bool AS relforcerowsecurity, "
-                          "0 AS relfrozenxid, 0 AS relminmxid,"
-                          "0 AS toid, "
-                          "0 AS tfrozenxid, 0 AS tminmxid,"
-                          "'p' AS relpersistence, 't' as relispopulated, "
-                          "'d' AS relreplident, relpages, "
-                          "NULL AS amname, "
-                          "NULL AS foreignserver, "
-                          "NULL AS reloftype, "
-                          "d.refobjid AS owning_tab, "
-                          "d.refobjsubid AS owning_col, "
-                          "(SELECT spcname FROM pg_tablespace t WHERE t.oid = c.reltablespace) AS reltablespace, "
-                          "NULL AS reloptions, "
-                          "NULL AS toast_reloptions, "
-                          "NULL AS changed_acl, "
-                          "NULL AS partkeydef, "
-                          "false AS ispartition, "
-                          "NULL AS partbound "
-                          "FROM pg_class c "
-                          "LEFT JOIN pg_depend d ON "
-                          "(c.relkind = '%c' AND "
-                          "d.classid = c.tableoid AND d.objid = c.oid AND "
-                          "d.objsubid = 0 AND "
-                          "d.refclassid = c.tableoid AND d.deptype = 'i') "
-                          "WHERE relkind in ('%c', '%c', '%c', '%c') "
-                          "ORDER BY c.oid",
-                          username_subquery,
-                          RELKIND_SEQUENCE,
-                          RELKIND_RELATION, RELKIND_SEQUENCE,
-                          RELKIND_VIEW, RELKIND_COMPOSITE_TYPE);
-    }
+        appendPQExpBufferStr(query,
+                             "c.relacl, NULL as rrelacl, "
+                             "NULL AS initrelacl, NULL AS initrrelacl, "
+                             "false AS changed_acl, ");
+
+    if (fout->remoteVersion >= 100000)
+        appendPQExpBufferStr(query,
+                             "pg_get_partkeydef(c.oid) AS partkeydef, "
+                             "c.relispartition AS ispartition, "
+                             "pg_get_expr(c.relpartbound, c.oid) AS partbound ");
+    else
+        appendPQExpBufferStr(query,
+                             "NULL AS partkeydef, "
+                             "false AS ispartition, "
+                             "NULL AS partbound ");
+
+    /*
+     * Left join to pg_depend to pick up dependency info linking sequences to
+     * their owning column, if any (note this dependency is AUTO as of 8.2).
+     * Also join to pg_tablespace to collect the spcname.
+     */
+    appendPQExpBufferStr(query,
+                         "\nFROM pg_class c\n"
+                         "LEFT JOIN pg_depend d ON "
+                         "(c.relkind = " CppAsString2(RELKIND_SEQUENCE) " AND "
+                         "d.classid = c.tableoid AND d.objid = c.oid AND "
+                         "d.objsubid = 0 AND "
+                         "d.refclassid = c.tableoid AND d.deptype IN ('a', 'i'))\n"
+                         "LEFT JOIN pg_tablespace tsp ON (tsp.oid = c.reltablespace)\n");
+
+    /*
+     * In 9.6 and up, left join to pg_init_privs to detect if any privileges
+     * are still as-set-at-init, in which case we won't dump out ACL commands
+     * for those.  We also are interested in the amname as of 9.6.
+     */
+    if (fout->remoteVersion >= 90600)
+        appendPQExpBufferStr(query,
+                             "LEFT JOIN pg_init_privs pip ON "
+                             "(c.oid = pip.objoid "
+                             "AND pip.classoid = 'pg_class'::regclass "
+                             "AND pip.objsubid = 0)\n"
+                             "LEFT JOIN pg_am am ON (c.relam = am.oid)\n");
+
+    /*
+     * We don't need any data from the TOAST table before 8.2.
+     *
+     * We purposefully ignore toast OIDs for partitioned tables; the reason is
+     * that versions 10 and 11 have them, but later versions do not, so
+     * emitting them causes the upgrade to fail.
+     */
+    if (fout->remoteVersion >= 80200)
+        appendPQExpBufferStr(query,
+                             "LEFT JOIN pg_class tc ON (c.reltoastrelid = tc.oid AND c.relkind <> "
CppAsString2(RELKIND_PARTITIONED_TABLE)")\n"); 
+
+    /*
+     * Restrict to interesting relkinds (in particular, not indexes).  Not all
+     * relkinds are possible in older servers, but it's not worth the trouble
+     * to emit a version-dependent list.
+     *
+     * Composite-type table entries won't be dumped as such, but we have to
+     * make a DumpableObject for them so that we can track dependencies of the
+     * composite type (pg_depend entries for columns of the composite type
+     * link to the pg_class entry not the pg_type entry).
+     */
+    appendPQExpBufferStr(query,
+                         "WHERE c.relkind IN ("
+                         CppAsString2(RELKIND_RELATION) ", "
+                         CppAsString2(RELKIND_SEQUENCE) ", "
+                         CppAsString2(RELKIND_VIEW) ", "
+                         CppAsString2(RELKIND_COMPOSITE_TYPE) ", "
+                         CppAsString2(RELKIND_MATVIEW) ", "
+                         CppAsString2(RELKIND_FOREIGN_TABLE) ", "
+                         CppAsString2(RELKIND_PARTITIONED_TABLE) ")\n"
+                         "ORDER BY c.oid");

     res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);

@@ -6949,7 +6661,7 @@ getTables(Archive *fout, int *numTables)
         }
         tblinfo[i].reltablespace = pg_strdup(PQgetvalue(res, i, i_reltablespace));
         tblinfo[i].reloptions = pg_strdup(PQgetvalue(res, i, i_reloptions));
-        if (i_checkoption == -1 || PQgetisnull(res, i, i_checkoption))
+        if (PQgetisnull(res, i, i_checkoption))
             tblinfo[i].checkoption = NULL;
         else
             tblinfo[i].checkoption = pg_strdup(PQgetvalue(res, i, i_checkoption));
@@ -6988,8 +6700,7 @@ getTables(Archive *fout, int *numTables)
         tblinfo[i].dummy_view = false;    /* might get set during sort */
         tblinfo[i].postponed_def = false;    /* might get set during sort */

-        tblinfo[i].is_identity_sequence = (i_is_identity_sequence >= 0 &&
-                                           strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0);
+        tblinfo[i].is_identity_sequence = (strcmp(PQgetvalue(res, i, i_is_identity_sequence), "t") == 0);

         /* Partition key string or NULL */
         tblinfo[i].partkeydef = pg_strdup(PQgetvalue(res, i, i_partkeydef));

Re: Refactoring pg_dump's getTables()

From
Alvaro Herrera
Date:
On 2021-Oct-16, Tom Lane wrote:

> Attached is a proposed patch that refactors getTables() along the
> same lines as some previous work (eg 047329624, ed2c7f65b, daa9fe8a5)
> to avoid having multiple partially-redundant copies of the SQL query.
> This gets rid of nearly 300 lines of duplicative spaghetti code,
> creates a uniform style for dealing with cross-version changes
> (replacing at least three different methods currently being used
> for that in this same stretch of code), and allows moving some
> comments to be closer to the code they describe.

Yeah, this seems a lot better than the original coding.  Maybe I would
group together the changes that all require the same version test,
rather than keeping the output columns in the same order.  This reduces
the number of branches.  Because the follow-on code uses column names
rather than numbers, there is no reason to keep related columns
together.  But it's a clear improvement even without that.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"



Re: Refactoring pg_dump's getTables()

From
Tom Lane
Date:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Yeah, this seems a lot better than the original coding.  Maybe I would
> group together the changes that all require the same version test,
> rather than keeping the output columns in the same order.  This reduces
> the number of branches.  Because the follow-on code uses column names
> rather than numbers, there is no reason to keep related columns
> together.  But it's a clear improvement even without that.

Yeah, I thought about rearranging the code order some more, but
desisted since it'd make the patch footprint a bit bigger (I'd
want to make all the related stanzas list things in a uniform
order).  But maybe we should just do that.

            regards, tom lane



Re: Refactoring pg_dump's getTables()

From
Daniel Gustafsson
Date:
> On 17 Oct 2021, at 22:05, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2021-Oct-16, Tom Lane wrote:
>
>> Attached is a proposed patch that refactors getTables() along the
>> same lines as some previous work (eg 047329624, ed2c7f65b, daa9fe8a5)
>> to avoid having multiple partially-redundant copies of the SQL query.
>> This gets rid of nearly 300 lines of duplicative spaghetti code,
>> creates a uniform style for dealing with cross-version changes
>> (replacing at least three different methods currently being used
>> for that in this same stretch of code), and allows moving some
>> comments to be closer to the code they describe.
>
> Yeah, this seems a lot better than the original coding.

+1

> Maybe I would group together the changes that all require the same version
> test, rather than keeping the output columns in the same order.


I agree with that, if we're doing all this we might as well go all the way for
readability.

--
Daniel Gustafsson        https://vmware.com/




Re: Refactoring pg_dump's getTables()

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
> On 17 Oct 2021, at 22:05, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Maybe I would group together the changes that all require the same version
>> test, rather than keeping the output columns in the same order.

> I agree with that, if we're doing all this we might as well go all the way for
> readability.

OK, I'll make it so.

            regards, tom lane



Re: Refactoring pg_dump's getTables()

From
Tom Lane
Date:
Daniel Gustafsson <daniel@yesql.se> writes:
> On 17 Oct 2021, at 22:05, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Maybe I would group together the changes that all require the same version
>> test, rather than keeping the output columns in the same order.

> I agree with that, if we're doing all this we might as well go all the way for
> readability.

I had a go at doing that, but soon decided that it wasn't as great an
idea as it first seemed.  There are two problems:

1. It's not clear what to do with fields where there are three or more
variants, such as reloptions and checkoptions.

2. Any time we modify the behavior for a particular field, we'd
have to merge or un-merge it from the stanza for the
previously-most-recently-relevant version.  This seems like it'd
be a maintenance headache and make patch footprints bigger than they
needed to be.

So I ended up not doing very much merging.  I did make an effort
to group the fields in perhaps a slightly more logical order
than before.

            regards, tom lane