Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Tue, Jun 23, 2026 at 5:20 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> All the above changes have been addressed in patch v9.
>

Thanks for the patches. A few ocmments:

1)

+ * In binary-upgrade mode, skip origin creation here. This is required to
+ * preserve the roident from the old cluster for this subscription.

subscription --> subscription's origin

2)
replorigin_create_with_id():

+ if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists", roname));

Can this ever happen? IIUC, since new-cluster should not have any
origin (as per the check introduced) and old cluster can not have
duplicate names, we should not hit this error. Do we want to keep it
as sanity check? If so, can we put a comment atop it.

3)
replorigin_create_with_id:

+ if (remote_lsn != InvalidXLogRecPtr)
+ replorigin_advance(roident, remote_lsn, InvalidXLogRecPtr,
+ false /* backward */,
+ false /* WAL log */);

We do not want to call 'advance' from regular 'replorigin_create'
flow. It is only for binary-Upgrade flow. Do you think we should have
a sanity check here?

if (remote_lsn != InvalidXLogRecPtr)
{
Assert(IsBinaryUpgrade)
replorigin_advance(..)
}

4)
replorigin_create:

+ found = true;

+ if (!found)
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("could not find free replication origin ID")));

Do you think we can make use of 'collides' itself here instead of
adding a new 'found' variable. We can move 'collides' outside of loop
and use it to emit above error.

5)
binary_upgrade_create_replication_origin:

+ node = (ReplOriginId) node_oid;
+
+ if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(node)))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists",
+ (Oid) node));

How can we hit this? Same comment as #2.

6)
-check_new_cluster_subscription_configuration(void)
+check_new_cluster_replication_origins(void)

- /* Quick return if there are no subscriptions to be migrated. */
- if (old_cluster.nsubs == 0)
- return;

- if (old_cluster.nsubs > max_active_replication_origins)

With these changes, do we even need to capture old-cluster's 'nsubs'
in get_subscription_info(). I do not see any other usage of nsubs. Let
me know if I am missing any case.

thanks
Shveta