On Wed, Jul 22, 2026 at 11:24 AM Ajin Cherian <[email protected]> wrote: > > On Tue, Jul 21, 2026 at 9:04 PM Shlok Kyal <[email protected]> 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
