Sort DO_SUBSCRIPTION_REL dump objects independent of OIDs.

Commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6 missed
DO_SUBSCRIPTION_REL, leading to assertion failures.  In the unlikely use
case of diffing "pg_dump --binary-upgrade" output, spurious diffs were
possible.  As part of fixing that, align the DumpableObject naming and
sort order with DO_PUBLICATION_REL.  The overall effect of this commit
is to change sort order from (subname, srsubid) to (rel, subname).
Since DO_SUBSCRIPTION_REL is only for --binary-upgrade, accept that
larger-than-usual dump order change.  Back-patch to v17, where commit
9a17be1e244a45a77de25ed2ada246fd34e4557d introduced DO_SUBSCRIPTION_REL.

Reported-by: vignesh C <[email protected]>
Author: vignesh C <[email protected]>
Discussion: https://postgr.es/m/CALDaNm2x3rd7C0_HjUpJFbxpAqXgm=QtoKfkEWDVA8h+JFpa_w@mail.gmail.com
Backpatch-through: 17

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 24ad201af2f986c5dce990edd71a5724254ce8b0..27f6be3f0f82bed1511ce5a922763a084517a483 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5386,7 +5386,9 @@ getSubscriptionRelations(Archive *fout)
        subrinfo[i].dobj.catId.tableoid = relid;
        subrinfo[i].dobj.catId.oid = cur_srsubid;
        AssignDumpId(&subrinfo[i].dobj);
-       subrinfo[i].dobj.name = pg_strdup(subinfo->dobj.name);
+       subrinfo[i].dobj.namespace = tblinfo->dobj.namespace;
+       subrinfo[i].dobj.name = tblinfo->dobj.name;
+       subrinfo[i].subinfo = subinfo;
        subrinfo[i].tblinfo = tblinfo;
        subrinfo[i].srsubstate = PQgetvalue(res, i, i_srsubstate)[0];
        if (PQgetisnull(res, i, i_srsublsn))
@@ -5394,8 +5396,6 @@ getSubscriptionRelations(Archive *fout)
        else
            subrinfo[i].srsublsn = pg_strdup(PQgetvalue(res, i, i_srsublsn));
 
-       subrinfo[i].subinfo = subinfo;
-
        /* Decide whether we want to dump it */
        selectDumpableObject(&(subrinfo[i].dobj), fout);
    }
@@ -5423,7 +5423,7 @@ dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
 
    Assert(fout->dopt->binary_upgrade && fout->remoteVersion >= 170000);
 
-   tag = psprintf("%s %s", subinfo->dobj.name, subrinfo->dobj.name);
+   tag = psprintf("%s %s", subinfo->dobj.name, subrinfo->tblinfo->dobj.name);
 
    query = createPQExpBuffer();
 
@@ -5438,7 +5438,7 @@ dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
                             "\n-- For binary upgrade, must preserve the subscriber table.\n");
        appendPQExpBufferStr(query,
                             "SELECT pg_catalog.binary_upgrade_add_sub_rel_state(");
-       appendStringLiteralAH(query, subrinfo->dobj.name, fout);
+       appendStringLiteralAH(query, subinfo->dobj.name, fout);
        appendPQExpBuffer(query,
                          ", %u, '%c'",
                          subrinfo->tblinfo->dobj.catId.oid,

diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 164c76e086406f41d0417e57da244b81024f97f4..e2a4df4cf4b6cc8dd60d0471d8f6849a4d1b5535 100644 (file)
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -454,6 +454,17 @@ DOTypeNameCompare(const void *p1, const void *p2)
        if (cmpval != 0)
            return cmpval;
    }
+   else if (obj1->objType == DO_SUBSCRIPTION_REL)
+   {
+       SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
+       SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
+
+       /* Sort by subscription name, since (namespace, name) match the rel */
+       cmpval = strcmp(srobj1->subinfo->dobj.name,
+                       srobj2->subinfo->dobj.name);
+       if (cmpval != 0)
+           return cmpval;
+   }
 
    /*
     * Shouldn't get here except after catalog corruption, but if we do, sort

diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl
index 77387be0f9d56dff8dfb1caa1e13e60f7d6bf5d7..fdb9806713e3dd3fa57120661e4b858218c45a1c 100644 (file)
--- a/src/bin/pg_upgrade/t/004_subscription.pl
+++ b/src/bin/pg_upgrade/t/004_subscription.pl
@@ -250,22 +250,25 @@ rmtree($new_sub->data_dir . "/pg_upgrade_output.d");
 # Verify that the upgrade should be successful with tables in 'ready'/'init'
 # state along with retaining the replication origin's remote lsn,
 # subscription's running status, failover option, and retain_dead_tuples
-# option.
+# option. Use multiple tables to verify deterministic pg_dump ordering
+# of subscription relations during --binary-upgrade.
 $publisher->safe_psql(
    'postgres', qq[
+       CREATE TABLE tab_upgraded(id int);
        CREATE TABLE tab_upgraded1(id int);
-       CREATE PUBLICATION regress_pub4 FOR TABLE tab_upgraded1;
+       CREATE PUBLICATION regress_pub4 FOR TABLE tab_upgraded, tab_upgraded1;
 ]);
 
 $old_sub->safe_psql(
    'postgres', qq[
+       CREATE TABLE tab_upgraded(id int);
        CREATE TABLE tab_upgraded1(id int);
        CREATE SUBSCRIPTION regress_sub4 CONNECTION '$connstr' PUBLICATION regress_pub4 WITH (failover = true, retain_dead_tuples = true);
 ]);
 
-# Wait till the table tab_upgraded1 reaches 'ready' state
+# Wait till the tables tab_upgraded and tab_upgraded1 reach 'ready' state
 my $synced_query =
-  "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'r'";
+  "SELECT count(1) = 2 FROM pg_subscription_rel WHERE srsubstate = 'r'";
 $old_sub->poll_query_until('postgres', $synced_query)
   or die "Timed out while waiting for the table to reach ready state";
 
@@ -303,6 +306,8 @@ my $remote_lsn = $old_sub->safe_psql('postgres',
 # Have the subscription in disabled state before upgrade
 $old_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub5 DISABLE");
 
+my $tab_upgraded_oid = $old_sub->safe_psql('postgres',
+   "SELECT oid FROM pg_class WHERE relname = 'tab_upgraded'");
 my $tab_upgraded1_oid = $old_sub->safe_psql('postgres',
    "SELECT oid FROM pg_class WHERE relname = 'tab_upgraded1'");
 my $tab_upgraded2_oid = $old_sub->safe_psql('postgres',
@@ -317,10 +322,10 @@ $new_sub->append_conf('postgresql.conf',
 
 # ------------------------------------------------------
 # Check that pg_upgrade is successful when all tables are in ready or in
-# init state (tab_upgraded1 table is in ready state and tab_upgraded2 table is
-# in init state) along with retaining the replication origin's remote lsn,
-# subscription's running status, failover option, and retain_dead_tuples
-# option.
+# init state (tab_upgraded and tab_upgraded1 tables are in ready state and
+# tab_upgraded2 table is in init state) along with retaining the replication
+# origin's remote lsn, subscription's running status, failover option, and
+# retain_dead_tuples option.
 # ------------------------------------------------------
 command_ok(
    [
@@ -369,9 +374,10 @@ regress_sub5|f|f|f),
 # Subscription relations should be preserved
 $result = $new_sub->safe_psql('postgres',
    "SELECT srrelid, srsubstate FROM pg_subscription_rel ORDER BY srrelid");
-is( $result, qq($tab_upgraded1_oid|r
+is( $result, qq($tab_upgraded_oid|r
+$tab_upgraded1_oid|r
 $tab_upgraded2_oid|i),
-   "there should be 2 rows in pg_subscription_rel(representing tab_upgraded1 and tab_upgraded2)"
+   "there should be 3 rows in pg_subscription_rel(representing tab_upgraded, tab_upgraded1 and tab_upgraded2)"
 );
 
 # The replication origin's remote_lsn should be preserved