pg_dump crash due to incomplete ordering of DO_SUBSCRIPTION_REL objects
Hi,
While verifying upgrade of subscriber instance, I noticed pg_dump
crash caused by incomplete sorting logic for DO_SUBSCRIPTION_REL
objects in DOTypeNameCompare(). When multiple subscription–relation
entries belong to the same subscription, the comparison does not
establish a complete ordering. In this case, the comparison falls
through to the generic assertion path. The attached patch fixes this
by extending the comparison for DO_SUBSCRIPTION_REL objects to include
deterministic ordering keys. After the subscription name comparison,
entries are ordered by the referenced table's schema name and then by
table name.
This issue has started failing after commit:
commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6
Sort dump objects independent of OIDs, for the 7 holdout object types.
This can be reproduced by having logical replication setup with
subscription subscribing to few tables.
Thanks,
Vignesh
Attachments:
0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchapplication/octet-stream; name=0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchDownload
From 1f8db153985a3e22738f7406477f80970f5c7091 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 15 Dec 2025 23:27:40 +0530
Subject: [PATCH] Fix pg_dump crash for DO_SUBSCRIPTION_REL sorting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
pg_dump did not fully order DO_SUBSCRIPTION_REL objects. When multiple
subscription–relation entries belonged to the same subscription, the comparison
fell through to the assertion path and crashed.
Fix this by extending the comparison to order such entries by the referenced
table's schema name and table name.
---
src/bin/pg_dump/pg_dump_sort.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 164c76e0864..4a02e1da8b0 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -454,6 +454,20 @@ 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 schema name (subscription name was already considered) */
+ cmpval = strcmp(srobj1->tblinfo->dobj.namespace->dobj.name,
+ srobj2->tblinfo->dobj.namespace->dobj.name);
+ if (cmpval != 0)
+ return cmpval;
+
+ /* Sort by table name */
+ return strcmp(srobj1->tblinfo->dobj.name, srobj2->tblinfo->dobj.name);
+ }
/*
* Shouldn't get here except after catalog corruption, but if we do, sort
--
2.43.0
On Mon, Dec 15, 2025 at 11:35:35PM +0530, vignesh C wrote:
While verifying upgrade of subscriber instance, I noticed pg_dump
crash caused by incomplete sorting logic for DO_SUBSCRIPTION_REL
objects in DOTypeNameCompare(). When multiple subscription–relation
entries belong to the same subscription, the comparison does not
establish a complete ordering. In this case, the comparison falls
through to the generic assertion path. The attached patch fixes this
by extending the comparison for DO_SUBSCRIPTION_REL objects to include
deterministic ordering keys. After the subscription name comparison,
entries are ordered by the referenced table's schema name and then by
table name.This issue has started failing after commit:
commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6
Sort dump objects independent of OIDs, for the 7 holdout object types.This can be reproduced by having logical replication setup with
subscription subscribing to few tables.
That makes sense. Thanks. Do you have commands we could add to
src/test/regress/sql/subscription.sql to cover this code?
On Tue, 16 Dec 2025 at 00:00, Noah Misch <noah@leadboat.com> wrote:
On Mon, Dec 15, 2025 at 11:35:35PM +0530, vignesh C wrote:
While verifying upgrade of subscriber instance, I noticed pg_dump
crash caused by incomplete sorting logic for DO_SUBSCRIPTION_REL
objects in DOTypeNameCompare(). When multiple subscription–relation
entries belong to the same subscription, the comparison does not
establish a complete ordering. In this case, the comparison falls
through to the generic assertion path. The attached patch fixes this
by extending the comparison for DO_SUBSCRIPTION_REL objects to include
deterministic ordering keys. After the subscription name comparison,
entries are ordered by the referenced table's schema name and then by
table name.This issue has started failing after commit:
commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6
Sort dump objects independent of OIDs, for the 7 holdout object types.This can be reproduced by having logical replication setup with
subscription subscribing to few tables.That makes sense. Thanks. Do you have commands we could add to
src/test/regress/sql/subscription.sql to cover this code?
This dumping of subscription relation is specific to upgrading to
preserve the subscription relation. So I felt we will not be able to
add tests to subscription.sql, instead how about adding one more table
to 004_subscription.pl where subscription upgrade tests are verified
like the attached patch.
Regards,
Vignesh
Attachments:
v2-0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchtext/x-patch; charset=UTF-8; name=v2-0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchDownload
From 41f8e1933bc0e71642ed2d717801527a118ed924 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 15 Dec 2025 23:27:40 +0530
Subject: [PATCH v2] Fix pg_dump crash for DO_SUBSCRIPTION_REL sorting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
pg_dump did not fully order DO_SUBSCRIPTION_REL objects. When multiple
subscription–relation entries belonged to the same subscription, the comparison
fell through to the assertion path and crashed.
Fix this by extending the comparison to order such entries by the referenced
table's schema name and table name.
---
src/bin/pg_dump/pg_dump_sort.c | 14 ++++++++++++++
src/bin/pg_upgrade/t/004_subscription.pl | 18 ++++++++++++------
2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c
index 164c76e0864..4a02e1da8b0 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -454,6 +454,20 @@ 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 schema name (subscription name was already considered) */
+ cmpval = strcmp(srobj1->tblinfo->dobj.namespace->dobj.name,
+ srobj2->tblinfo->dobj.namespace->dobj.name);
+ if (cmpval != 0)
+ return cmpval;
+
+ /* Sort by table name */
+ return strcmp(srobj1->tblinfo->dobj.name, srobj2->tblinfo->dobj.name);
+ }
/*
* 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 77387be0f9d..71203b8ed03 100644
--- 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 reaches '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',
@@ -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
--
2.43.0
On Tue, Dec 16, 2025 at 05:22:36PM +0530, vignesh C wrote:
On Tue, 16 Dec 2025 at 00:00, Noah Misch <noah@leadboat.com> wrote:
On Mon, Dec 15, 2025 at 11:35:35PM +0530, vignesh C wrote:
This issue has started failing after commit:
commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6
Sort dump objects independent of OIDs, for the 7 holdout object types.
That makes sense. Thanks. Do you have commands we could add to
src/test/regress/sql/subscription.sql to cover this code?This dumping of subscription relation is specific to upgrading to
preserve the subscription relation. So I felt we will not be able to
add tests to subscription.sql, instead how about adding one more table
to 004_subscription.pl where subscription upgrade tests are verified
like the attached patch.
That's a good way to test it.
--- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -454,6 +454,20 @@ 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 schema name (subscription name was already considered) */
Let's change the values getSubscriptionRelations() stores in
SubrRelInfo.obj.namespace and SubrRelInfo.obj.name to be more like
DO_PUBLICATION_REL:
pubrinfo[j].dobj.namespace = tbinfo->dobj.namespace;
pubrinfo[j].dobj.name = tbinfo->dobj.name;
pubrinfo[j].publication = pubinfo;
DO_SUBSCRIPTION_REL (new in v17) is gratuitously different:
subrinfo[i].dobj.name = pg_strdup(subinfo->dobj.name);
subrinfo[i].tblinfo = tblinfo;
subrinfo[i].subinfo = subinfo;
What do you think? This would change sort order from (subname, rel) to (rel,
subname). Historically, we've avoided churning dump order, in case folks diff
dump output over time. I bet that practice is less common for binary dumps.
Since DO_SUBSCRIPTION_REL is only for binary dumps, let's just change it.
+ cmpval = strcmp(srobj1->tblinfo->dobj.namespace->dobj.name, + srobj2->tblinfo->dobj.namespace->dobj.name);
The subrinfo change will make this comparison go away. If it had stayed, it
should be ready for namespace==NULL like pgTypeNameCompare() is. (I think
pgTypeNameCompare() could drop that defense, because the getTypes() call to
findNamespace() will pg_fatal() on absence. Until it does drop that defense,
the rest of pg_dump_sort.c should handle namespace==NULL.)
--- a/src/bin/pg_upgrade/t/004_subscription.pl +++ b/src/bin/pg_upgrade/t/004_subscription.pl
-# Wait till the table tab_upgraded1 reaches 'ready' state +# Wait till the tables tab_upgraded and tab_upgraded1 reaches 'ready' state
s/reaches/reach/ there.
To find other text needing edits, I searched this file for tab_upgraded. The
following comment still implies "all tables" encompasses just two:
# ------------------------------------------------------
# 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.
# ------------------------------------------------------
On Wed, 17 Dec 2025 at 00:54, Noah Misch <noah@leadboat.com> wrote:
On Tue, Dec 16, 2025 at 05:22:36PM +0530, vignesh C wrote:
On Tue, 16 Dec 2025 at 00:00, Noah Misch <noah@leadboat.com> wrote:
On Mon, Dec 15, 2025 at 11:35:35PM +0530, vignesh C wrote:
This issue has started failing after commit:
commit 0decd5e89db9f5edb9b27351082f0d74aae7a9b6
Sort dump objects independent of OIDs, for the 7 holdout object types.That makes sense. Thanks. Do you have commands we could add to
src/test/regress/sql/subscription.sql to cover this code?This dumping of subscription relation is specific to upgrading to
preserve the subscription relation. So I felt we will not be able to
add tests to subscription.sql, instead how about adding one more table
to 004_subscription.pl where subscription upgrade tests are verified
like the attached patch.That's a good way to test it.
--- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -454,6 +454,20 @@ 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 schema name (subscription name was already considered) */Let's change the values getSubscriptionRelations() stores in
SubrRelInfo.obj.namespace and SubrRelInfo.obj.name to be more like
DO_PUBLICATION_REL:pubrinfo[j].dobj.namespace = tbinfo->dobj.namespace;
pubrinfo[j].dobj.name = tbinfo->dobj.name;
pubrinfo[j].publication = pubinfo;DO_SUBSCRIPTION_REL (new in v17) is gratuitously different:
subrinfo[i].dobj.name = pg_strdup(subinfo->dobj.name);
subrinfo[i].tblinfo = tblinfo;
subrinfo[i].subinfo = subinfo;
Modified
What do you think? This would change sort order from (subname, rel) to (rel,
subname). Historically, we've avoided churning dump order, in case folks diff
dump output over time.
Although grouping by publication and subscription would be cleaner,
publication relations have been ordered first by relation and then by
publication. Retaining the same ordering for subscription relations
preserves consistency.
I bet that practice is less common for binary dumps.
Since DO_SUBSCRIPTION_REL is only for binary dumps, let's just change it.+ cmpval = strcmp(srobj1->tblinfo->dobj.namespace->dobj.name, + srobj2->tblinfo->dobj.namespace->dobj.name);The subrinfo change will make this comparison go away. If it had stayed, it
should be ready for namespace==NULL like pgTypeNameCompare() is. (I think
pgTypeNameCompare() could drop that defense, because the getTypes() call to
findNamespace() will pg_fatal() on absence. Until it does drop that defense,
the rest of pg_dump_sort.c should handle namespace==NULL.)--- a/src/bin/pg_upgrade/t/004_subscription.pl +++ b/src/bin/pg_upgrade/t/004_subscription.pl-# Wait till the table tab_upgraded1 reaches 'ready' state +# Wait till the tables tab_upgraded and tab_upgraded1 reaches 'ready' states/reaches/reach/ there.
Modified
To find other text needing edits, I searched this file for tab_upgraded. The
following comment still implies "all tables" encompasses just two:# ------------------------------------------------------
# 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.
# ------------------------------------------------------
Updated the comments
The attached v3 version patch has the changes for the same.
Regards,
Vignesh
Attachments:
v3-0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchapplication/x-patch; name=v3-0001-Fix-pg_dump-crash-for-DO_SUBSCRIPTION_REL-sorting.patchDownload
From 5b3c16301be655f8205f3f5308696aa785375493 Mon Sep 17 00:00:00 2001
From: Vignesh C <vignesh21@gmail.com>
Date: Mon, 15 Dec 2025 23:27:40 +0530
Subject: [PATCH v3] Fix pg_dump crash for DO_SUBSCRIPTION_REL sorting
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
pg_dump did not fully order DO_SUBSCRIPTION_REL objects. When multiple
subscription–relation entries belonged to the same subscription, the comparison
fell through to the assertion path and crashed.
Fix this by extending the comparison to order such entries by the referenced
table's schema name and table name.
---
src/bin/pg_dump/pg_dump.c | 8 ++++----
src/bin/pg_dump/pg_dump_sort.c | 10 +++++++++
src/bin/pg_upgrade/t/004_subscription.pl | 26 +++++++++++++++---------
3 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 24ad201af2f..0f3f41c10b4 100644
--- 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);
}
@@ -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 164c76e0864..661d933e8aa 100644
--- a/src/bin/pg_dump/pg_dump_sort.c
+++ b/src/bin/pg_dump/pg_dump_sort.c
@@ -454,6 +454,16 @@ 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 77387be0f9d..fdb9806713e 100644
--- 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
--
2.43.0
On Wed, Dec 17, 2025 at 10:11:58AM +0530, vignesh C wrote:
The attached v3 version patch has the changes for the same.
The "tag" variable needed a change to compensate for the subrinfo->dobj.name
change. I plan to push the attached version.
Attachments:
DO_SUBSCRIPTION_REL-v4.patchtext/plain; charset=us-asciiDownload
commit d15c9e5 (HEAD, zzy_test-commit-master)
Author: Noah Misch <noah@leadboat.com>
AuthorDate: Wed Dec 17 11:18:00 2025 -0800
Commit: Noah Misch <noah@leadboat.com>
CommitDate: Wed Dec 17 11:18:00 2025 -0800
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 <vignesh21@gmail.com>
Author: vignesh C <vignesh21@gmail.com>
Discussion: https://postgr.es/m/CALDaNm2x3rd7C0_HjUpJFbxpAqXgm=QtoKfkEWDVA8h+JFpa_w@mail.gmail.com
Backpatch-through: 17
---
src/bin/pg_dump/pg_dump.c | 10 +++++-----
src/bin/pg_dump/pg_dump_sort.c | 11 +++++++++++
src/bin/pg_upgrade/t/004_subscription.pl | 26 ++++++++++++++++----------
3 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 24ad201..27f6be3 100644
--- 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 164c76e..e2a4df4 100644
--- 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 77387be..fdb9806 100644
--- 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
On Thu, 18 Dec 2025 at 01:21, Noah Misch <noah@leadboat.com> wrote:
On Wed, Dec 17, 2025 at 10:11:58AM +0530, vignesh C wrote:
The attached v3 version patch has the changes for the same.
The "tag" variable needed a change to compensate for the subrinfo->dobj.name
change. I plan to push the attached version.
Thanks, the changes look good.
Regards,
Vignesh
On Dec 18, 2025, at 03:51, Noah Misch <noah@leadboat.com> wrote:
On Wed, Dec 17, 2025 at 10:11:58AM +0530, vignesh C wrote:
The attached v3 version patch has the changes for the same.
The "tag" variable needed a change to compensate for the subrinfo->dobj.name
change. I plan to push the attached version.
<DO_SUBSCRIPTION_REL-v4.patch>
v4 looks solid. A couple of nitpicks:
1
```
+ SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
+ SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
```These two temp pointers can be const, like:
```
const SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
const SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
```
2
```
+ /* Sort by subscription name, since (namespace, name) match the rel */
```
This comment is correct, but sounds a little insider-ish. Maybe:
/* Tiebreak by subscription name; (namespace, name) already identify the table */
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Thu, 18 Dec 2025 at 14:05, Chao Li <li.evan.chao@gmail.com> wrote:
On Dec 18, 2025, at 03:51, Noah Misch <noah@leadboat.com> wrote:
On Wed, Dec 17, 2025 at 10:11:58AM +0530, vignesh C wrote:
The attached v3 version patch has the changes for the same.
The "tag" variable needed a change to compensate for the subrinfo->dobj.name
change. I plan to push the attached version.
<DO_SUBSCRIPTION_REL-v4.patch>v4 looks solid. A couple of nitpicks:
1 ``` + SubRelInfo *srobj1 = *(SubRelInfo *const *) p1; + SubRelInfo *srobj2 = *(SubRelInfo *const *) p2; ```These two temp pointers can be const, like:
```
const SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
const SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
```
I felt the way it is handled in the patch is ok and consistent with
the other variables used in this function.
2
```
+ /* Sort by subscription name, since (namespace, name) match the rel */
```This comment is correct, but sounds a little insider-ish. Maybe:
/* Tiebreak by subscription name; (namespace, name) already identify the table */
Similarly here too, it is inline with similar comments of other enums
in this function.
Regards,
Vignesh
On Thu, Dec 18, 2025 at 05:35:45PM +0530, vignesh C wrote:
On Thu, 18 Dec 2025 at 14:05, Chao Li <li.evan.chao@gmail.com> wrote:
On Dec 18, 2025, at 03:51, Noah Misch <noah@leadboat.com> wrote:
I plan to push the attached version.
<DO_SUBSCRIPTION_REL-v4.patch>
Pushed as d49936f etc.
2
```
+ /* Sort by subscription name, since (namespace, name) match the rel */
```This comment is correct, but sounds a little insider-ish. Maybe:
/* Tiebreak by subscription name; (namespace, name) already identify the table */
Similarly here too, it is inline with similar comments of other enums
in this function.
Exactly. For cosmetics, consistency with nearby code is the stronger rule.