Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Wed, Jul 22, 2026 at 11:24 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> On Tue, Jul 21, 2026 at 9:04 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> >
> > 2. I was testing the patch and found a difference of behaviour between
> > HEAD and with Patch.
> > Suppose we have two nodes, publisher and subscriber, and the
> > subscriber has two subscriptions. The replication origin states for
> > the respective subscriptions are:
> > local_id | external_id | remote_lsn | local_lsn
> > ----------+-------------+------------+------------
> > 1 | pg_16393 | 0/00000000 | 0/0174F518
> > 2 | pg_16394 | 0/017516C0 | 0/0174FDA0
> > Now, we want to upgrade the subscriber node. On the
> > upgraded_subscriber node, max_logical_replication_workers is set to 0.
> >
> > With this patch, the replication origin states after the upgrade are:
> > local_id | external_id | remote_lsn | local_lsn
> > ----------+-------------+------------+------------
> > 2 | pg_16394 | 0/017516C0 | 0/00000000
> > (1 row)
> >
> > Whereas on HEAD, the replication origin states after the upgrade are:
> > local_id | external_id | remote_lsn | local_lsn
> > ----------+-------------+------------+------------
> > 1 | pg_16400 | 0/00000000 | 0/00000000
> > 2 | pg_16401 | 0/017516C0 | 0/00000000
> >
> > With this patch, I observed that replication origins whose remote_lsn
> > is '0/00000000' are not present on the upgraded node when
> > max_logical_replication_workers is set to 0.
> > Is this behaviour expected?
> >
>
> The reason for this difference in behaviour is that on head,
> replorigin_advance is called unconditionally even if remote_lsn = 0/0,
> which is even if the origin hasn't replayed anything (e.g.,
> subscription hasn't started replicating, or table sync hasn't
> progressed past the initial state). So, the origin has an entry in
> pg_replication_status. (Should it?)
> With the patch, although the replication origin is created, it is
> advanced at upgrade time only if it has a meaningful remote_lsn and
> otherwise it will eventually get updated when the apply worker starts
> up. But, in the test scenario, the apply worker never gets a chance to
> run because max_logical_replication_workers is set to 0. I think this
> behaviour is fine. But if maintaining consistency with previous
> versions is important, I also understand that.
> The particular code that causes this difference is:
>
> + if (remote_lsn != InvalidXLogRecPtr)
> + {
> + /*
> + * 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.
> + */
> + Assert(IsBinaryUpgrade);
> +
> + replorigin_advance(roident, remote_lsn, InvalidXLogRecPtr,
> + false /* backward */,
> + false /* WAL log */);
> + }
>
> The code does not differentiate between a remote_lsn of 0/0 and the
> caller passing InvalidXLogRecPtr. I could revert to older behaviour by
> changing the if condition to if (IsBinaryUpgrade).
>
> Let me know what people think? Keep the existing behaviour or is the
> new behaviour fine?
>

I don't have a strong opinion on this.

The case we are trying to address is an extreme corner case (and, IMO,
rather impractical), so I am fine with leaving the behavior as is.
Changing the check would also work. But the only downside if at all in
the future, someone else uses replorigin_create_with_id() and also
wants to set remote_lsn, we may need to keep adding more conditions,
since checking only IsBinaryUpgrade may not be sufficient. The
current condition is more generic and seems cleaner to me.

thanks
Shveta