On Tue, Dec 16, 2025 at 05:22:36PM +0530, vignesh C wrote:
> 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:
> > > 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.
# ------------------------------------------------------
