Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Thu, Jul 30, 2026 at 12:47 PM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:
>
> Hi Ajin,
>
> Thanks -- 0/0 lines up now, and the pg_upgrade.c comment is fixed too. But
> `if (IsBinaryUpgrade)` also advances origins that were never tracked.

Yeah, I see the problem.

> Old cluster, max_active_replication_origins = 1. The second origin can be
> created because it is never tracked:
>
> SELECT pg_replication_origin_create('advanced_one');
> SELECT pg_replication_origin_advance('advanced_one', '0/1000');
> SELECT pg_replication_origin_create('unadvanced_one');
>
> SELECT * FROM pg_replication_origin;
> roident | roname
> ---------+----------------
> 1 | advanced_one
> 2 | unadvanced_one
>
> SELECT * FROM pg_replication_origin_status;
> local_id | external_id | remote_lsn | local_lsn
> ----------+--------------+------------+------------
> 1 | advanced_one | 0/00001000 | 0/00000000
>
> Upgrading to a new cluster with the same max_active_replication_origins = 1:
>
> Performing Consistency Checks
> ...
> Checking replication origins in new cluster ok
> Performing Upgrade
> ...
> Restoring global objects in the new cluster
> *failure*
>
> ERROR: could not find free replication state slot for replication origin
> with ID 2
>
> The check passed because it counted the one status row; the restore then wanted
> two slots. And it gets there after "If pg_upgrade fails after this point, you
> must re-initdb the new cluster before continuing.", so a worse landing place
> than the refusal I reported on v13.
>
> HEAD guards this in the dumpSubscription() path the patch replaces:
>
> if (subinfo->suboriginremotelsn)
>
> NULL exactly when there was no status row, so HEAD advances iff the origin was
> tracked, 0/0 included. dumpReplicationOrigins() still draws the same line --
>
> if (!PQgetisnull(res, i, i_remotelsn))
>
> emits the lsn, and NULL otherwise -- so the distinction survives the dump; it
> just gets dropped in the backend, where a missing LSN and a real 0/0 are both
> InvalidXLogRecPtr by the time replorigin_create_with_id() sees them. An
> untracked origin then starts counting against max_active_replication_origins,
> which is what counting status rows was meant to avoid.
>
> The caller still knows which is which -- NULL vs '0/0' -- so passing it down:
>
> void
> replorigin_create_with_id(ReplOriginId roident, const char *roname,
> - XLogRecPtr remote_lsn, Relation rel)
> + XLogRecPtr remote_lsn, bool need_advance, Relation rel)
> @@
> - if (IsBinaryUpgrade)
> + if (need_advance)
> {
> - /* If part of a binary upgrade, advance the origin as well */
> + Assert(IsBinaryUpgrade);
> replorigin_advance(roident, remote_lsn, InvalidXLogRecPtr,
> @@ replorigin_create()
> - replorigin_create_with_id(roident, roname, InvalidXLogRecPtr, rel);
> + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr, false, rel);
> @@ binary_upgrade_create_replication_origin()
> - replorigin_create_with_id(node, originname, remote_lsn, rel);
> + replorigin_create_with_id(node, originname, remote_lsn,
> + !PG_ARGISNULL(2), rel);
>
> reproduces the old cluster's tracking exactly: the case above upgrades, and an
> origin sitting at 0/0 still keeps its status row.
>

I agree with the problem and the solution.

The origins which were not tracked on old node should not be tracked
(and should not occupy a slot) on new node. And the origins which were
tracked on old node should be tracked even at 0x0. And through
remote_lsn, replorigin_create_with_id() can not distinguish these
cases as remote_lsn will be InvalidXLogRecPtr for both the cases. So
passing the argument seems a reasonable solution here.

> 004_subscription.pl already creates a user origin and never advances it, so it
> only wanted an assertion that the origin is still untracked afterwards; that
> one fails on v14 and passes with the change. Attached is the whole thing, code
> and test, on top of v14 -- the pg_upgrade suite and the main regression tests
> are green with it.
>
> The other way to close the gap is to leave the advance alone and count all
> origins in the check instead. That works too, but it makes pg_upgrade ask for a
> max_active_replication_origins the server itself doesn't require -- untracked
> origins are free to create today -- so matching the old cluster's tracking
> looked like the smaller change.
>

We had discussed this kind of solution earlier (for a different
problem) and decided against it, as it will need user to set
unnecessarily high 'max_active_replication_origins' on new cluster. I
prefer the solution you provided in the patch. Thanks for working on
this.

thanks
Shveta