On Tue, 16 Dec 2025 at 00:00, Noah Misch <[email protected]> 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
From 41f8e1933bc0e71642ed2d717801527a118ed924 Mon Sep 17 00:00:00 2001 From: Vignesh C <[email protected]> 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
