Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Mon, Sep 28, 2026 at 5:51 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Addressed these comments and also rebased the patch.
>

Hi Ajin,

I reviewed and tested v18. Please find my comments below:

1) When upgrading from PG16 or older, a subscription can end up
without its replication origin after the upgrade.

In binary-upgrade mode, CreateSubscription() now skips
replorigin_create(), relying on pg_dumpall to restore the origin. That
only works if the origin exists on the old cluster. For PG17+ sources,
check_old_cluster_subscription_state() rejects subscriptions without
an origin, but that check isn't done for older versions. So if the
subscription's origin was dropped on a PG16 cluster (rare, since it
requires a manual pg_replication_origin_drop()), nothing creates it
during the upgrade.

Reproduced with a PG16 old cluster (subscription created with connect
= false, then its origin dropped):

HEAD + v18:
new cluster: subscription oid 16384, origins: []
ALTER SUBSCRIPTION regress_sub SKIP (lsn = '0/1000000');
ERROR: replication origin "pg_16384" does not exist

HEAD:
new cluster: subscription oid 16400, origins: [1:pg_16400]
ALTER SUBSCRIPTION regress_sub SKIP (lsn = '0/1000000');
ALTER SUBSCRIPTION
-- on head, CreateSubscription() simply creats a new origin.

The apply worker creates the missing origin once the subscription is
enabled, so this is temporary, but until then commands that need the
origin fail. I think this is a leftover of the case Shlok reported in
[1], which was fixed by dumping origins for all source versions [3].
That fix doesn't cover an origin that's already missing on the old
cluster.

Since dropping a subscription's origin is rare, a simple fix might be
enough: in binary-upgrade mode, create the origin only if it doesn't
already exist, e.g.

- if (!IsBinaryUpgrade)
- {
- ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
+ ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
+ if (!IsBinaryUpgrade || !OidIsValid(replorigin_by_name(originname, true)))
replorigin_create(originname);
- }

Thoughts?
~~~

Tests: 004_subscription.pl:

2) The subscription roident check passes on HEAD, i.e. without the
patch, so it cannot catch a regression:

+my %all_subnames = (%pre_upgrade_roident, %post_upgrade_roident);
+for my $subname (sort keys %all_subnames)
+{
+ next if $subname eq $user_origin_name;
+ is($post_upgrade_roident{$subname}, $pre_upgrade_roident{$subname},
+ "roident preserved for subscription '$subname' after upgrade");
+}

Result on HEAD with the v18 test applied (regress_log_004_subscription snippet):

[15:01:58.310](0.157s) not ok 14 - subscription oid should have been preserved
[15:01:58.310](0.000s)
[15:01:58.310](0.000s) # Failed test 'subscription oid should have
been preserved'
# at t/004_subscription.pl line 416.
[15:01:58.310](0.000s) # got: '16400
# 16401'
# expected: '16402
# 16406'
[15:01:58.321](0.012s) ok 15 - check that the subscription's running
status, failover, and retain_dead_tuples are preserved
[15:01:58.334](0.012s) ok 16 - roident preserved for subscription
'regress_sub4' after upgrade
[15:01:58.334](0.000s) ok 17 - roident preserved for subscription
'regress_sub5' after upgrade
[15:01:58.345](0.011s) not ok 18 - roident preserved for user-created
origin 'regress_user_origin' after upgrade
[15:01:58.345](0.000s)
[15:01:58.345](0.000s) # Failed test 'roident preserved for
user-created origin 'regress_user_origin' after upgrade'
# at t/004_subscription.pl line 461.
[15:01:58.345](0.000s) # got: ''
# expected: '3'
```

The subscription OIDs did change (test 14), yet the roidents still
compare equal. Origins get the lowest free ID, and by this point every
earlier subscription and its origin has been dropped. So the old
cluster ends up with IDs:
regress_sub4 = 1
regress_sub5 = 2
regress_user_origin = 3
regress_zero_origin = 4
Without the patch, the new cluster recreates the subscription origins
in dump order (sub4, then sub5) and gets 1 and 2 again. Only tests 18
and 20 catch a full revert, because the user-created origins are not
migrated at all on HEAD. A partial breakage would pass every check:
for example, origins restored but given fresh IDs instead of the old
roident.

How about leaving a gap, so that a fresh assignment gives different
numbers? Something like:

# before CREATE SUBSCRIPTION regress_sub4
$old_sub->safe_psql('postgres',
"SELECT pg_replication_origin_create('regress_filler')");
...
# after all origins are created, i.e. after regress_zero_origin
$old_sub->safe_psql('postgres',
"SELECT pg_replication_origin_drop('regress_filler')");

The old cluster then has sub4 = 2, sub5 = 3. Any fresh assignment
gives 1, 2 so the check can actually fail.

3) Similarly, this check passes on HEAD, but only because the origin
does not exist at all:

+is($result, qq(0),
+ "never-advanced origin '$user_origin_name' is still untracked after upgrad
+);
ok 19 - never-advanced origin 'regress_user_origin' is still untracked
after upgrade

The join to pg_replication_origin by name returns 0 rows whether the
origin is untracked or missing. It would be better to check that the
origin exists and has no status row, e.g. a LEFT JOIN from
pg_replication_origin that expects one row with a NULL local_id.
~~~

4) Almost all files need pgindent to be run.

5) v18-0001 does not compile cleanly on its own:

subscriptioncmds.c:95:7: warning: no previous extern declaration for
non-static variable 'binary_upgrade_next_pg_subscription_oid'
[-Wmissing-variable-declarations]
95 | Oid
binary_upgrade_next_pg_subscription_oid = InvalidOid;
|

The #include "catalog/binary_upgrade.h" in subscriptioncmds.c is added
in 0002; it should be moved into 0001.

[1] https://www.postgresql.org/message-id/CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAA4eK1KzGSANd%3Dg1S8xpcyiwh3M-c%2BUaa4%2B447v-%2BN8chX7K3g%40mail.gmail.com

--
Thanks,
Nisha