Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Thu, Jul 16, 2026 at 11:36 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Fri, Jul 10, 2026 at 6:28 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> >
> > Thanks,
> > Shlok Kyal
>
> Thanks for all the comments. Here's an updated patch addressing these comments.
>

Thanks Ajin. A few trivial comments:

1)
+ configured to a value greater than or equal to the number of active
+ replication origins present in the old cluster. The new cluster
must contain no replication origins.

Suggestion:
configured to a value greater than or equal to the number of tracked
replication origins present in the old cluster (reflected in
pg_replication_origin_status)

(active -->tracked, link to pg_replication_origin_status at the end)

2)
+ * restoring its remote_lsn. Used by pg_upgrade to preserve replication
+ * origin IDs across the upgrade.

Shall we get rid of 'Used by pg_upgrade to pre..' as it is used by
other flow as well.

3)

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

We can instead add a comment atop Assert here (remove comment atop
function header as suggested in 2)

/*
* The remote_lsn is expected to be valid only during binary upgrade when
* preserving an existing replication origin. For the normal origin
* creation flow, it should always be InvalidXLogRecPtr.
*/

4)

- * preserved during the upgrade. So, even after creating the slot, the
- * upgraded subscriber may be unable to detect conflicts or log relevant
- * commit timestamps and origins when applying changes from the publisher
- * occurred before the upgrade especially if those changes were not
- * replicated.
+ * upgrade. So, even after creating the slot, the upgraded subscriber may
+ * be unable to detect conflicts or log relevant commit timestamps when
+ * applying changes from the publisher occurred before the upgrade
+ * especially if those changes were not replicated.

I think, we should retain 'origins' in above comment as we still can
not log origin info post-upgrade until commit-timestamp is preserved.

5)
# ------------------------------------------------------
# Check that the data inserted to the publisher when the new subscriber is down
# will be replicated once it is started. Also check that the old subscription
# states and relations origins are all preserved, and that the conflict
# detection slot is created.
# ------------------------------------------------------

We can change this existing comment in test in accordance with new
origin-id presrevation test.

6)
# Verify that the subscription related replication origins are
preserved after upgrade.
# Verify that user created replication origins are preserved after upgrade.

Shall we say:
# Verify that subscription replication origins retain their origin IDs
after upgrade.
# Verify that the user-created replication origin retains its origin
ID after upgrade.

thanks
Shveta