Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Introduce replication progress tracking infrastructure.
- 5aa2350426c4 9.5.0 cited

[PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-04-28T11:19:38Z

Hello hackers,

The idea for this patch came up during discussions in the thread [1]
on migration of the pg_commit_ts directory as part of pg_upgrade.
There was a problem raised by Sawada-san in that thread which this
patch addresses. [2]

The problem:
The pg_commit_ts directory stores commit-timestamp records for each
transaction, and each record embeds the replication origin ID
(roident) that identifies which subscription wrote that transaction.
When pg_upgrade migrates a subscriber, the pg_commit_ts directory is
copied directly from the old cluster to the new cluster. This means
those embedded roidents must remain valid in the new cluster. When
pg_upgrade migrates a subscriber, CREATE SUBSCRIPTION on the new
cluster calls replorigin_create() which assigns fresh roidents to each
subscription's replication origin. Because subscription OIDs are not
stable across upgrades, the origin names change (e.g. pg_16392 becomes
pg_16403), and consequently the roidents can be assigned differently —
or in the worst case, swapped between subscriptions.

Consider two subscriptions subA and subB with roidents 1 and 2
respectively before upgrade. After upgrade, due to OID reassignment,
subA might get roident 2 and subB might get roident 1. The
commit-timestamp records copied from the old cluster still say roident
1 for rows written by subA, but the new cluster now thinks roident 1
belongs to subB. This causes spurious update_origin_differs conflicts
— the new cluster incorrectly thinks a row was last modified by a
different subscription than it actually was.

This patch attempts to fix this by replicating the roident of the
replication origins of each subscription on migration. This patch also
migrates all replication origins as part of pg_upgrade.

Sequence of Events During Upgrade

1. pg_dumpall dumps all non-subscription replication origins from the
old cluster with their roidents and LSN positions.
2. pg_dump dumps each subscription, but now records the old roident
alongside the subscription info.
3. During restore, pg_dumpall's output recreates non-subscription
origins on the new cluster with their original roidents via
binary_upgrade_create_replication_origin().
4. During per-database restore, CREATE SUBSCRIPTION runs but skips
origin creation.
5. binary_upgrade_set_next_replorigin_oid() creates the origin for
each subscription with the preserved roident.
6. binary_upgrade_replorigin_advance() restores the LSN position for
each subscription.
7. Subscriptions that were running before upgrade are re-enabled.

Please let me know your feedback regarding this patch

[1] - https://www.postgresql.org/message-id/flat/182311743703924%40mail.yandex.ru
[2] - https://www.postgresql.org/message-id/CAD21AoDG8zQpHHfw7OvaEy7W0ZSyP%3D_dS-hrcquJ3C_ctMDmMQ%40mail.gmail.com

regards,
Ajin Cherian
Fujitsu Australia

RE: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-04-29T08:40:21Z

Dear Ajin,

> Sequence of Events During Upgrade
> 
> 1. pg_dumpall dumps all non-subscription replication origins from the
> old cluster with their roidents and LSN positions.
> 2. pg_dump dumps each subscription, but now records the old roident
> alongside the subscription info.
> 3. During restore, pg_dumpall's output recreates non-subscription
> origins on the new cluster with their original roidents via
> binary_upgrade_create_replication_origin().

To confirm, why do we have to handle separately for subscription-associated
origins? I'm thinking it's not needed if the subscription's OID is preserved
during the upgrade. 

I checked the old thread to preserve it [1], but it could not be accepted because
there are no strong motivations. But I feel this is the good reason to do so now.

How do you feel?

[1]: https://www.postgresql.org/message-id/CALDaNm2Wj63VcbB0SY2NECHr1mKM1YSaV1ZydrdQVxyox2O2hg%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

vignesh C <vignesh21@gmail.com> — 2026-04-30T06:51:54Z

On Wed, 29 Apr 2026 at 14:11, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Ajin,
>
> > Sequence of Events During Upgrade
> >
> > 1. pg_dumpall dumps all non-subscription replication origins from the
> > old cluster with their roidents and LSN positions.
> > 2. pg_dump dumps each subscription, but now records the old roident
> > alongside the subscription info.
> > 3. During restore, pg_dumpall's output recreates non-subscription
> > origins on the new cluster with their original roidents via
> > binary_upgrade_create_replication_origin().
>
> To confirm, why do we have to handle separately for subscription-associated
> origins? I'm thinking it's not needed if the subscription's OID is preserved
> during the upgrade.

+1 to preserve the subscription OID. This should make preserving
replication origin easier.

> I checked the old thread to preserve it [1], but it could not be accepted because
> there are no strong motivations. But I feel this is the good reason to do so now.

Here is a rebased version of the patch.

Regards,
Vignesh

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-04-30T09:37:08Z

On Wed, Apr 29, 2026 at 2:11 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Ajin,
>
> > Sequence of Events During Upgrade
> >
> > 1. pg_dumpall dumps all non-subscription replication origins from the
> > old cluster with their roidents and LSN positions.
> > 2. pg_dump dumps each subscription, but now records the old roident
> > alongside the subscription info.
> > 3. During restore, pg_dumpall's output recreates non-subscription
> > origins on the new cluster with their original roidents via
> > binary_upgrade_create_replication_origin().
>
> To confirm, why do we have to handle separately for subscription-associated
> origins? I'm thinking it's not needed if the subscription's OID is preserved
> during the upgrade.
>

I’m not sure how preserving the subscription OID would ensure that the
origin ID is also preserved for sub-associated origins. Could you
please elaborate?

As I understand it, roident values are assigned independently during
origin creation. Even if subscription OIDs are preserved, the origin
IDs could still be reassigned differently on the new cluster. For
example, suppose we have two subscriptions, sub1 and sub2, with
roident values 2 and 3, assuming 1 was previously used and dropped.
After upgrade, origin creation may start allocating from 1 again,
resulting in roident values 1 and 2 instead. Since pg_commit_ts stores
the numeric roident, not the origin name, this mismatch could still
lead to incorrect conflict detection. Wouldn’t that result in the same
wrong conflict detection issue we are trying to avoid?
Please let me know if my understanding is wrong.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-06T05:38:00Z

On Thu, Apr 30, 2026 at 4:52 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Wed, 29 Apr 2026 at 14:11, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Ajin,
> >
> > > Sequence of Events During Upgrade
> > >
> > > 1. pg_dumpall dumps all non-subscription replication origins from the
> > > old cluster with their roidents and LSN positions.
> > > 2. pg_dump dumps each subscription, but now records the old roident
> > > alongside the subscription info.
> > > 3. During restore, pg_dumpall's output recreates non-subscription
> > > origins on the new cluster with their original roidents via
> > > binary_upgrade_create_replication_origin().
> >
> > To confirm, why do we have to handle separately for subscription-associated
> > origins? I'm thinking it's not needed if the subscription's OID is preserved
> > during the upgrade.
>
> +1 to preserve the subscription OID. This should make preserving
> replication origin easier.
>
> > I checked the old thread to preserve it [1], but it could not be accepted because
> > there are no strong motivations. But I feel this is the good reason to do so now.
>
> Here is a rebased version of the patch.

Thanks Vignesh for the patch. I have used your patch as 0001 and
created mine on top of that as 0002. Like Kuroda-san said, with your
patch, I no longer need to have special handling of subscription
replication origins when pg_dumpall creates all replication origins on
the new cluster as now the name of origin is also guaranteed to be the
same because the replication origin name is created using the oid of
the subscription which is now the same because of the the changes in
patch 0001.
Here's v3 with the updated changes.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-06T05:43:02Z

On Thu, Apr 30, 2026 at 7:37 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> I’m not sure how preserving the subscription OID would ensure that the
> origin ID is also preserved for sub-associated origins. Could you
> please elaborate?
>
> As I understand it, roident values are assigned independently during
> origin creation. Even if subscription OIDs are preserved, the origin
> IDs could still be reassigned differently on the new cluster. For
> example, suppose we have two subscriptions, sub1 and sub2, with
> roident values 2 and 3, assuming 1 was previously used and dropped.
> After upgrade, origin creation may start allocating from 1 again,
> resulting in roident values 1 and 2 instead. Since pg_commit_ts stores
> the numeric roident, not the origin name, this mismatch could still
> lead to incorrect conflict detection. Wouldn’t that result in the same
> wrong conflict detection issue we are trying to avoid?
> Please let me know if my understanding is wrong.

In the first patch, the replication origins were duplicated from the
old cluster to the new with matching roidents and ronames. This
couldn't be done for subscription replication origins as subscriptions
weren't preserving OIDs on the new cluster and therefore the
corresponding roname which is derived from the subscription OIDs also
differed. Now with matching roname and roident, all the replication
origins from the old cluster can be copied over to the new cluster in
one shot.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-05-06T10:43:41Z

On Wed, May 6, 2026 at 11:13 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Thu, Apr 30, 2026 at 7:37 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > I’m not sure how preserving the subscription OID would ensure that the
> > origin ID is also preserved for sub-associated origins. Could you
> > please elaborate?
> >
> > As I understand it, roident values are assigned independently during
> > origin creation. Even if subscription OIDs are preserved, the origin
> > IDs could still be reassigned differently on the new cluster. For
> > example, suppose we have two subscriptions, sub1 and sub2, with
> > roident values 2 and 3, assuming 1 was previously used and dropped.
> > After upgrade, origin creation may start allocating from 1 again,
> > resulting in roident values 1 and 2 instead. Since pg_commit_ts stores
> > the numeric roident, not the origin name, this mismatch could still
> > lead to incorrect conflict detection. Wouldn’t that result in the same
> > wrong conflict detection issue we are trying to avoid?
> > Please let me know if my understanding is wrong.
>
> In the first patch, the replication origins were duplicated from the
> old cluster to the new with matching roidents and ronames. This
> couldn't be done for subscription replication origins as subscriptions
> weren't preserving OIDs on the new cluster and therefore the
> corresponding roname which is derived from the subscription OIDs also
> differed.

Okay. I think you did not post the first patch you are referring to
here. V2 was posted directly. But I see your point.

> Now with matching roname and roident, all the replication
> origins from the old cluster can be copied over to the new cluster in
> one shot.

Okay.  Will review the patch.

thanks
Shveta

RE: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-05-07T07:47:01Z

Dear Ajin,

Thanks for updating the patch. Let me share my two high-level comments.

1.
Can you clarify the policy for backward compatibility? In other words, should we
preserve subscription OIDs and migrate replication origins from PG19- instances?
Similar commits 9a17be1 and 29d0a77 did not allow migrating objects from released
versions.

2.
I found that other objects use global variables like binary_upgrade_next_xxx to
specify the next OID. Can we follow the manner even for replication origins?
Or it's not a good approach because of some reasons?

BTW, cfbot got angry with your patch.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-07T21:06:07Z

Hello!

+ appendPQExpBuffer(buf,
+   "SELECT pg_catalog.binary_upgrade_create_replication_origin('%u'::pg_catalog.oid,
'%s'::pg_catalog.name",
+   roident, roname);
+
+ if (!PQgetisnull(res, i, i_remotelsn))
+ {
+ appendPQExpBuffer(buf, ", '%s'::pg_catalog.pg_lsn",
+   PQgetvalue(res, i, i_remotelsn));
+ }
+ else

Isn't some string escaping missing from this? It doesn't seem like the
most likely SQL injection target, but could still cause surprises.
Could be even verified by a test case using
pg_replication_origin_create('O''Brien_replica')

- if (old_cluster.nsubs > max_active_replication_origins)
+ if (old_cluster.nrepl_origins > max_active_replication_origins)
  pg_fatal("\"max_active_replication_origins\" (%d) must be greater
than or equal to the number of "
  "subscriptions (%d) on the old cluster",
- max_active_replication_origins, old_cluster.nsubs);
+ max_active_replication_origins, old_cluster.nrepl_origins);

This error message could be misleading now, it's not the number of
subscriptions but the number of replication origins.

+ rel = table_open(ReplicationOriginRelationId, ExclusiveLock);

Do we need an ExclusiveLock, couldn't this instead use a
RowExclusiveLock? In the unlikely situation that another session
inserts the same record, the unique index should be able catch it?


+#include "access/xlogdefs.h"

This seems to be unnecessary?

- appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn);
+ if (fout->remoteVersion < 190000 && subinfo->suboriginremotelsn)
+ {

subinfo->suboriginremotelsn is already checked by the outer if

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-11T12:06:24Z

On Thu, May 7, 2026 at 5:47 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Ajin,
>
> Thanks for updating the patch. Let me share my two high-level comments.
>
> 1.
> Can you clarify the policy for backward compatibility? In other words, should we
> preserve subscription OIDs and migrate replication origins from PG19- instances?
> Similar commits 9a17be1 and 29d0a77 did not allow migrating objects from released
> versions.

I thought about this and after discussions with others decided that we
should support migrating replication_origins from PG17 onwards. Prior
to that pg_subscription_rel and remote LSN were not updated as part of
migration. It's important that we support migration of versions prior
to PG_19, otherwise we will end up skipping creating replication
origins as part of CREATE SUBSCRIPTION in the new cluster and
subscriptions will be without replication origins. So, I have updated
the check to migrate replication origins as long as the old cluster is
PG17 or greater.

>
> 2.
> I found that other objects use global variables like binary_upgrade_next_xxx to
> specify the next OID. Can we follow the manner even for replication origins?
> Or it's not a good approach because of some reasons?
>

This naming convention is followed for preserving subscription OIDs
but since for replication origins we are creating each replication
origin with the same roname and roident, I don't think the same
situation applies here.

> BTW, cfbot got angry with your patch.

Yes, fixed that.


On Fri, May 8, 2026 at 7:06 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> Hello!
>
> + appendPQExpBuffer(buf,
> +   "SELECT pg_catalog.binary_upgrade_create_replication_origin('%u'::pg_catalog.oid,
> '%s'::pg_catalog.name",
> +   roident, roname);
> +
> + if (!PQgetisnull(res, i, i_remotelsn))
> + {
> + appendPQExpBuffer(buf, ", '%s'::pg_catalog.pg_lsn",
> +   PQgetvalue(res, i, i_remotelsn));
> + }
> + else
>
> Isn't some string escaping missing from this? It doesn't seem like the
> most likely SQL injection target, but could still cause surprises.
> Could be even verified by a test case using
> pg_replication_origin_create('O''Brien_replica')
>

Fixed.

> - if (old_cluster.nsubs > max_active_replication_origins)
> + if (old_cluster.nrepl_origins > max_active_replication_origins)
>   pg_fatal("\"max_active_replication_origins\" (%d) must be greater
> than or equal to the number of "
>   "subscriptions (%d) on the old cluster",
> - max_active_replication_origins, old_cluster.nsubs);
> + max_active_replication_origins, old_cluster.nrepl_origins);
>
> This error message could be misleading now, it's not the number of
> subscriptions but the number of replication origins.
>

Fixed.

> + rel = table_open(ReplicationOriginRelationId, ExclusiveLock);
>
> Do we need an ExclusiveLock, couldn't this instead use a
> RowExclusiveLock? In the unlikely situation that another session
> inserts the same record, the unique index should be able catch it?
>

Fixed.

>
> +#include "access/xlogdefs.h"
>
> This seems to be unnecessary?
>

It is required.

> - appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn);
> + if (fout->remoteVersion < 190000 && subinfo->suboriginremotelsn)
> + {
>
> subinfo->suboriginremotelsn is already checked by the outer if

Fixed.

Attaching version 4 with the fixes.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Zsolt Parragi <zsolt.parragi@percona.com> — 2026-05-11T18:32:02Z

A few more minor comments:

binary_upgrade_replorigin_advance seems like dead code in the patch
now, it has no callers since the patch removes its only use.

+ ReplOriginId node;
....
+ node = PG_GETARG_OID(0);

ReplOriginId is uint16, this silently truncates it. Since this is a
generic callable function shouldn't there be at least a check about
it?

Also, seems like the same function
(binary_upgrade_create_replication_origin) locks-unlocks-locks
ReplicationOriginRelationId, that doesn't seem the best approach with
a single-use helper function? (first in
replorigin_create_with_reploriginid, then
binary_upgrade_create_replication_origin reaquires it)

+ if (PQntuples(res) > 0 && archDumpFormat == archNull)
+ fprintf(OPF, "--\n-- Replication Origins \n--\n\n");

The caller also checks the format, it is redundant.

+ /* Get replication origins in current database. */
+ appendPQExpBufferStr(buf,

Isn't pg_replication_origin a shared catalog?

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-13T01:57:53Z

On Tue, May 12, 2026 at 4:32 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> A few more minor comments:
>
> binary_upgrade_replorigin_advance seems like dead code in the patch
> now, it has no callers since the patch removes its only use.
>

Removed.

> + ReplOriginId node;
> ....
> + node = PG_GETARG_OID(0);
>
> ReplOriginId is uint16, this silently truncates it. Since this is a
> generic callable function shouldn't there be at least a check about
> it?

Added check.

>
> Also, seems like the same function
> (binary_upgrade_create_replication_origin) locks-unlocks-locks
> ReplicationOriginRelationId, that doesn't seem the best approach with
> a single-use helper function? (first in
> replorigin_create_with_reploriginid, then
> binary_upgrade_create_replication_origin reaquires it)

Rewrote the function and removed replorigin_create_with_reploriginid
to do all the work in binary_upgrade_create_replication_origin()
itself.

>
> + if (PQntuples(res) > 0 && archDumpFormat == archNull)
> + fprintf(OPF, "--\n-- Replication Origins \n--\n\n");
>
> The caller also checks the format, it is redundant.
>

Fixed.

> + /* Get replication origins in current database. */
> + appendPQExpBufferStr(buf,
>
> Isn't pg_replication_origin a shared catalog?

Fixed.

Also changed the definition of
binary_upgrade_create_replication_origin to proparallel => 'u', as
parallel execution was causing a failure on FreeBSD in cfbot.

regards,
Ajin Cherian
Funitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-18T10:42:39Z
```
Rebased the patch as it was no longer applying.

regards,
Ajin Cherian
Fujitsu Australia
```

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-05-22T09:46:24Z

On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
>
> Rebased the patch as it was no longer applying.
>
Hi Ajin,

I have started reviewing the patch. Here is my comment for v6-0002 patch:

Suppose we have a replication setup: publisher -> subscriber
and we are upgrading subscriber to subscriber_new.
And if initially 'subscriber_new' has a replication origin, upgrading
the cluster can error out.

Example:
We set up a logical replication between publisher node and subscriber node.

On subscriber node:
postgres=# SELECT * FROM pg_replication_origin;
 roident |  roname
---------+----------
       1 | pg_16393
(1 row)

And initially subscriber_new has a replication origin:
postgres=# select pg_replication_origin_create('myname');
 pg_replication_origin_create
------------------------------
                            1
(1 row)

postgres=# SELECT * FROM pg_replication_origin;
 roident | roname
---------+--------
       1 | myname
(1 row)

Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
node, we get an error:
```
SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
ERROR:  replication origin with ID 1 already exists
```

This error occurs in "Performing Upgrade" stage. Should we add a check
in the "Performing Consistency Checks" stage so that we don't need to
re-initdb the new cluster to perform the upgrade?
Maybe we can add a check similar to
check_new_cluster_replication_slots(), where pg_upgrade errors out if
the new cluster already contains replication origins. Thoughts?

Thanks,
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-05-22T10:27:32Z

On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > Rebased the patch as it was no longer applying.
> >
> Hi Ajin,
>
> I have started reviewing the patch. Here is my comment for v6-0002 patch:
>
> Suppose we have a replication setup: publisher -> subscriber
> and we are upgrading subscriber to subscriber_new.
> And if initially 'subscriber_new' has a replication origin, upgrading
> the cluster can error out.
>
> Example:
> We set up a logical replication between publisher node and subscriber node.
>
> On subscriber node:
> postgres=# SELECT * FROM pg_replication_origin;
>  roident |  roname
> ---------+----------
>        1 | pg_16393
> (1 row)
>
> And initially subscriber_new has a replication origin:
> postgres=# select pg_replication_origin_create('myname');
>  pg_replication_origin_create
> ------------------------------
>                             1
> (1 row)
>
> postgres=# SELECT * FROM pg_replication_origin;
>  roident | roname
> ---------+--------
>        1 | myname
> (1 row)
>
> Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
> node, we get an error:
> ```
> SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
> ERROR:  replication origin with ID 1 already exists
> ```
>
> This error occurs in "Performing Upgrade" stage. Should we add a check
> in the "Performing Consistency Checks" stage so that we don't need to
> re-initdb the new cluster to perform the upgrade?
> Maybe we can add a check similar to
> check_new_cluster_replication_slots(), where pg_upgrade errors out if
> the new cluster already contains replication origins. Thoughts?

+1. I had the same thought while reviewing the patch today. We should
have it unless there is a reason we have avoided it??

Few trivial comments:

1)

+#include "access/skey.h"
+#include "catalog/indexing.h"

pg_upgrade_support.c compiles without above.

2)
+ Assert(!OidIsValid(rel->rd_rel->reltoastrelid));

Is there a reason for this sanity check? I generally do not see a
Null-Toast table sanity check after every table_open.

3)

+
+ /* Dump replication origins */
+ if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
+ dumpReplicationOrigins(conn);

why the check is for PG17 specifically?

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-05-25T07:13:27Z

On Fri, May 22, 2026 at 3:57 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > Rebased the patch as it was no longer applying.
> > >
> > Hi Ajin,
> >
> > I have started reviewing the patch. Here is my comment for v6-0002 patch:
> >
> > Suppose we have a replication setup: publisher -> subscriber
> > and we are upgrading subscriber to subscriber_new.
> > And if initially 'subscriber_new' has a replication origin, upgrading
> > the cluster can error out.
> >
> > Example:
> > We set up a logical replication between publisher node and subscriber node.
> >
> > On subscriber node:
> > postgres=# SELECT * FROM pg_replication_origin;
> >  roident |  roname
> > ---------+----------
> >        1 | pg_16393
> > (1 row)
> >
> > And initially subscriber_new has a replication origin:
> > postgres=# select pg_replication_origin_create('myname');
> >  pg_replication_origin_create
> > ------------------------------
> >                             1
> > (1 row)
> >
> > postgres=# SELECT * FROM pg_replication_origin;
> >  roident | roname
> > ---------+--------
> >        1 | myname
> > (1 row)
> >
> > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
> > node, we get an error:
> > ```
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
> > ERROR:  replication origin with ID 1 already exists
> > ```
> >
> > This error occurs in "Performing Upgrade" stage. Should we add a check
> > in the "Performing Consistency Checks" stage so that we don't need to
> > re-initdb the new cluster to perform the upgrade?
> > Maybe we can add a check similar to
> > check_new_cluster_replication_slots(), where pg_upgrade errors out if
> > the new cluster already contains replication origins. Thoughts?
>
> +1. I had the same thought while reviewing the patch today. We should
> have it unless there is a reason we have avoided it??
>
> Few trivial comments:
>
> 1)
>
> +#include "access/skey.h"
> +#include "catalog/indexing.h"
>
> pg_upgrade_support.c compiles without above.
>
> 2)
> + Assert(!OidIsValid(rel->rd_rel->reltoastrelid));
>
> Is there a reason for this sanity check? I generally do not see a
> Null-Toast table sanity check after every table_open.
>
> 3)
>
> +
> + /* Dump replication origins */
> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
> + dumpReplicationOrigins(conn);
>
> why the check is for PG17 specifically?
>

One issue in 002:

binary_upgrade_create_replication_origin() has this:

+ originname = PG_GETARG_NAME(1);
+
+ roname_d = CStringGetTextDatum(NameStr(*originname));
+

We are getting origin-name (text) into Name-type which can not be more
than 64 bytes. So if an origin has name more than 64, it will end up
trimming the name post-upgrade.

I tried this:

Old-setup:
postgres=# SELECT
pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64');
 pg_replication_origin_create
------------------------------
                            1
postgres=# select * from pg_replication_origin;
 roident |                                roname
---------+----------------------------------------------------------------------
       1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64


Post-upgrade: name got trimmed to 64 length.
-------------------------
postgres=#  select * from pg_replication_origin;
 roident |                             roname
---------+-----------------------------------------------------------------
       1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-05-28T02:48:30Z

On Fri, May 22, 2026 at 8:27 PM shveta malik <shveta.malik@gmail.com> wrote:

> On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com>
> wrote:
> >
> > On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > Rebased the patch as it was no longer applying.
> > >
> > Hi Ajin,
> >
> > I have started reviewing the patch. Here is my comment for v6-0002 patch:
> >
> > Suppose we have a replication setup: publisher -> subscriber
> > and we are upgrading subscriber to subscriber_new.
> > And if initially 'subscriber_new' has a replication origin, upgrading
> > the cluster can error out.
> >
> > Example:
> > We set up a logical replication between publisher node and subscriber
> node.
> >
> > On subscriber node:
> > postgres=# SELECT * FROM pg_replication_origin;
> >  roident |  roname
> > ---------+----------
> >        1 | pg_16393
> > (1 row)
> >
> > And initially subscriber_new has a replication origin:
> > postgres=# select pg_replication_origin_create('myname');
> >  pg_replication_origin_create
> > ------------------------------
> >                             1
> > (1 row)
> >
> > postgres=# SELECT * FROM pg_replication_origin;
> >  roident | roname
> > ---------+--------
> >        1 | myname
> > (1 row)
> >
> > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
> > node, we get an error:
> > ```
> > SELECT
> pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> >
> psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
> > ERROR:  replication origin with ID 1 already exists
> > ```
> >
> > This error occurs in "Performing Upgrade" stage. Should we add a check
> > in the "Performing Consistency Checks" stage so that we don't need to
> > re-initdb the new cluster to perform the upgrade?
> > Maybe we can add a check similar to
> > check_new_cluster_replication_slots(), where pg_upgrade errors out if
> > the new cluster already contains replication origins. Thoughts?
>
> +1. I had the same thought while reviewing the patch today. We should
> have it unless there is a reason we have avoided it??
>

Fixed this. Added a new check for replication origins and if the new
cluster has any existing replication origins, then the check will fail.

>
> Few trivial comments:
>
> 1)
>
> +#include "access/skey.h"
> +#include "catalog/indexing.h"
>
> pg_upgrade_support.c compiles without above.
>
>
Removed.


> 2)
> + Assert(!OidIsValid(rel->rd_rel->reltoastrelid));
>
> Is there a reason for this sanity check? I generally do not see a
> Null-Toast table sanity check after every table_open.
>
>
Removed.


> 3)
>
> +
> + /* Dump replication origins */
> + if (server_version >= 170000 && binary_upgrade && archDumpFormat ==
> archNull)
> + dumpReplicationOrigins(conn);
>
> why the check is for PG17 specifically?
>
>
In PG17, we started migrating pg_subscription_rel and the remote LSN during
upgrades; prior to that, these were not migrated. Given that change, it
also makes sense to migrate replication origins from them. Otherwise, when
upgrading from PG17 to a later version, you could end up with a
subscription where pg_subscription_rel and the remote LSN are migrated, but
the corresponding replication origin is not created.



On Mon, May 25, 2026 at 5:13 PM shveta malik <shveta.malik@gmail.com> wrote:

>
> One issue in 002:
>
> binary_upgrade_create_replication_origin() has this:
>
> + originname = PG_GETARG_NAME(1);
> +
> + roname_d = CStringGetTextDatum(NameStr(*originname));
> +
>
> We are getting origin-name (text) into Name-type which can not be more
> than 64 bytes. So if an origin has name more than 64, it will end up
> trimming the name post-upgrade.
>
> I tried this:
>
> Old-setup:
> postgres=# SELECT
>
> pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64');
>  pg_replication_origin_create
> ------------------------------
>                             1
> postgres=# select * from pg_replication_origin;
>  roident |                                roname
>
> ---------+----------------------------------------------------------------------
>        1 |
> this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64
>
>
> Post-upgrade: name got trimmed to 64 length.
> -------------------------
> postgres=#  select * from pg_replication_origin;
>  roident |                             roname
> ---------+-----------------------------------------------------------------
>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li
>
> thanks
> Shveta


Fixed this. Now  binary_upgrade_create_replication_origin handles it
similarly to the way pg_replication_origin_create handles the name of the
origin.

Here's an updated version v7 containing these fixes.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

vignesh C <vignesh21@gmail.com> — 2026-06-05T10:09:49Z

On Thu, 28 May 2026 at 08:18, Ajin Cherian <itsajin@gmail.com> wrote:
>
> Fixed this. Added a new check for replication origins and if the new cluster has any existing replication origins, then the check will fail.
>>
>>
>> Few trivial comments:
>>
>> 1)
>>
>> +#include "access/skey.h"
>> +#include "catalog/indexing.h"
>>
>> pg_upgrade_support.c compiles without above.
>>
>
> Removed.
>
>>
>> 2)
>> + Assert(!OidIsValid(rel->rd_rel->reltoastrelid));
>>
>> Is there a reason for this sanity check? I generally do not see a
>> Null-Toast table sanity check after every table_open.
>>
>
> Removed.
>
>>
>> 3)
>>
>> +
>> + /* Dump replication origins */
>> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
>> + dumpReplicationOrigins(conn);
>>
>> why the check is for PG17 specifically?
>>
>
> In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
>
>
>
> On Mon, May 25, 2026 at 5:13 PM shveta malik <shveta.malik@gmail.com> wrote:
>>
>>
>> One issue in 002:
>>
>> binary_upgrade_create_replication_origin() has this:
>>
>> + originname = PG_GETARG_NAME(1);
>> +
>> + roname_d = CStringGetTextDatum(NameStr(*originname));
>> +
>>
>> We are getting origin-name (text) into Name-type which can not be more
>> than 64 bytes. So if an origin has name more than 64, it will end up
>> trimming the name post-upgrade.
>>
>> I tried this:
>>
>> Old-setup:
>> postgres=# SELECT
>> pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64');
>>  pg_replication_origin_create
>> ------------------------------
>>                             1
>> postgres=# select * from pg_replication_origin;
>>  roident |                                roname
>> ---------+----------------------------------------------------------------------
>>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64
>>
>>
>> Post-upgrade: name got trimmed to 64 length.
>> -------------------------
>> postgres=#  select * from pg_replication_origin;
>>  roident |                             roname
>> ---------+-----------------------------------------------------------------
>>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li
>>
>> thanks
>> Shveta
>
>
> Fixed this. Now  binary_upgrade_create_replication_origin handles it similarly to the way pg_replication_origin_create handles the name of the origin.

Few comments:
1) pg_dumpall dumps all replication origins, not just subscription
origins and recreates every origin:
SELECT o.*, os.remote_lsn
FROM pg_replication_origin o
LEFT JOIN pg_replication_origin_status os
  ON o.roident = os.local_id

Is pg_upgrade expected to preserve user-created origins?
If so, you can mention this in the commit message

2) Should the table lock be released here, if so should we mention
comments similar to how it is mentioned in
binary_upgarde_add_sub_rel_state:
+ /* Restore the remote_lsn if provided, while still holding the lock */
+ if (!PG_ARGISNULL(2))
+ {
+ XLogRecPtr remote_commit = PG_GETARG_LSN(2);

- ReplicationSlotRelease();
+ replorigin_advance(node, remote_commit, InvalidXLogRecPtr,
+    false /* backward */,
+    false /* WAL log */);
+ }
+
+ table_close(rel, RowExclusiveLock);

3) We can use new style of ereport to exclude '(' before errcode:
3.a)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("replication origin name is too long"),
+ errdetail("Replication origin names must be no longer than %d bytes.",
+    MAX_RONAME_LEN)));

3.b) Similarly here:
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists", node_oid)));

3.c) There are many others, you can search other ereport in the patch
and change it too.

4) Since only one column will be returned here, can we use
"PQgetvalue(res, 0, 0) and remove PQfnumber:
+ res = executeQueryOrDie(conn,
+ "SELECT count(*) AS nrepl_origins "
+ "FROM pg_catalog.pg_replication_origin");
+ i_nrepl_origins = PQfnumber(res, "nrepl_origins");
+ cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_nrepl_origins));
+ PQclear(res);

5) replorigin_create also has some duplicate code like below, will it
be possible to have a common function so that both of them can use?
/* Check for name collision */
+ ScanKeyInit(&key,
+ Anum_pg_replication_origin_roname,
+ BTEqualStrategyNumber, F_TEXTEQ,
+ roname_d);
+ scan = systable_beginscan(rel, ReplicationOriginNameIndex,
+   true /* indexOK */,
+   SnapshotSelf,
+   1, &key);
+ collides = HeapTupleIsValid(systable_getnext(scan));
+ systable_endscan(scan);
+
+ if (collides)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists",
+ originname)));
+
+ memset(&nulls, 0, sizeof(nulls));
+ memset(&values, 0, sizeof(values));
+
+ values[Anum_pg_replication_origin_roident - 1] = ObjectIdGetDatum(node);
+ values[Anum_pg_replication_origin_roname - 1] = roname_d;
+
+ tuple = heap_form_tuple(RelationGetDescr(rel), values, nulls);
+ CatalogTupleInsert(rel, tuple);
+ heap_freetuple(tuple);
+ CommandCounterIncrement();

Regards,
Vignesh

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-08T17:06:13Z

On Thu, 28 May 2026 at 08:18, Ajin Cherian <itsajin@gmail.com> wrote:
>
>
>
> On Fri, May 22, 2026 at 8:27 PM shveta malik <shveta.malik@gmail.com> wrote:
>>
>> On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>> >
>> > On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
>> > >
>> > > Rebased the patch as it was no longer applying.
>> > >
>> > Hi Ajin,
>> >
>> > I have started reviewing the patch. Here is my comment for v6-0002 patch:
>> >
>> > Suppose we have a replication setup: publisher -> subscriber
>> > and we are upgrading subscriber to subscriber_new.
>> > And if initially 'subscriber_new' has a replication origin, upgrading
>> > the cluster can error out.
>> >
>> > Example:
>> > We set up a logical replication between publisher node and subscriber node.
>> >
>> > On subscriber node:
>> > postgres=# SELECT * FROM pg_replication_origin;
>> >  roident |  roname
>> > ---------+----------
>> >        1 | pg_16393
>> > (1 row)
>> >
>> > And initially subscriber_new has a replication origin:
>> > postgres=# select pg_replication_origin_create('myname');
>> >  pg_replication_origin_create
>> > ------------------------------
>> >                             1
>> > (1 row)
>> >
>> > postgres=# SELECT * FROM pg_replication_origin;
>> >  roident | roname
>> > ---------+--------
>> >        1 | myname
>> > (1 row)
>> >
>> > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
>> > node, we get an error:
>> > ```
>> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
>> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
>> > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
>> > ERROR:  replication origin with ID 1 already exists
>> > ```
>> >
>> > This error occurs in "Performing Upgrade" stage. Should we add a check
>> > in the "Performing Consistency Checks" stage so that we don't need to
>> > re-initdb the new cluster to perform the upgrade?
>> > Maybe we can add a check similar to
>> > check_new_cluster_replication_slots(), where pg_upgrade errors out if
>> > the new cluster already contains replication origins. Thoughts?
>>
>> +1. I had the same thought while reviewing the patch today. We should
>> have it unless there is a reason we have avoided it??
>
>
> Fixed this. Added a new check for replication origins and if the new cluster has any existing replication origins, then the check will fail.
>>
>>
>> Few trivial comments:
>>
>> 1)
>>
>> +#include "access/skey.h"
>> +#include "catalog/indexing.h"
>>
>> pg_upgrade_support.c compiles without above.
>>
>
> Removed.
>
>>
>> 2)
>> + Assert(!OidIsValid(rel->rd_rel->reltoastrelid));
>>
>> Is there a reason for this sanity check? I generally do not see a
>> Null-Toast table sanity check after every table_open.
>>
>
> Removed.
>
>>
>> 3)
>>
>> +
>> + /* Dump replication origins */
>> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
>> + dumpReplicationOrigins(conn);
>>
>> why the check is for PG17 specifically?
>>
>
> In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
>
>
>
> On Mon, May 25, 2026 at 5:13 PM shveta malik <shveta.malik@gmail.com> wrote:
>>
>>
>> One issue in 002:
>>
>> binary_upgrade_create_replication_origin() has this:
>>
>> + originname = PG_GETARG_NAME(1);
>> +
>> + roname_d = CStringGetTextDatum(NameStr(*originname));
>> +
>>
>> We are getting origin-name (text) into Name-type which can not be more
>> than 64 bytes. So if an origin has name more than 64, it will end up
>> trimming the name post-upgrade.
>>
>> I tried this:
>>
>> Old-setup:
>> postgres=# SELECT
>> pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64');
>>  pg_replication_origin_create
>> ------------------------------
>>                             1
>> postgres=# select * from pg_replication_origin;
>>  roident |                                roname
>> ---------+----------------------------------------------------------------------
>>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64
>>
>>
>> Post-upgrade: name got trimmed to 64 length.
>> -------------------------
>> postgres=#  select * from pg_replication_origin;
>>  roident |                             roname
>> ---------+-----------------------------------------------------------------
>>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li
>>
>> thanks
>> Shveta
>
>
> Fixed this. Now  binary_upgrade_create_replication_origin handles it similarly to the way pg_replication_origin_create handles the name of the origin.
>
> Here's an updated version v7 containing these fixes.

Hi Ajin,
I have reviewed the patch and here are some comments:

1. I want to clarify, what is the expected behaviour when we upgrade
the postgres instance from version <= 16 to "HEAD + patch"?
-   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
-   replorigin_create(originname);
+   if (!IsBinaryUpgrade)
+   {
+       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
originname, sizeof(originname));
+       replorigin_create(originname);
+   }
and
+           /* Dump replication origins */
+           if (server_version >= 170000 && binary_upgrade &&
archDumpFormat == archNull)
+               dumpReplicationOrigins(conn);

When the postgres instance with subscription is updated from PG 16 (or
less) to HEAD, the replication origin is created as we do not have "if
(!IsBinaryUpgrade)" check in HEAD,
whereas when the similar instance is upgraded to "HEAD + patch", no
replication origin is present after the upgrade.
Is this difference in the behaviour between HEAD and "HEAD + patch" expected?

2.a) Can we replace:
+   /* Check for OID collision */
+   ScanKeyInit(&key,
+               Anum_pg_replication_origin_roident,
+               BTEqualStrategyNumber, F_OIDEQ,
+               ObjectIdGetDatum(node));
+   scan = systable_beginscan(rel, ReplicationOriginIdentIndex,
+                             true /* indexOK */,
+                             SnapshotSelf,
+                             1, &key);
+   collides = HeapTupleIsValid(systable_getnext(scan));
+   systable_endscan(scan);
+
+   if (collides)
+       ereport(ERROR,
+               (errcode(ERRCODE_DUPLICATE_OBJECT),
+                errmsg("replication origin with ID %u already
exists", node_oid)));

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

2.b) Similarly, Can we replace:
+   /* Check for name collision */
+   ScanKeyInit(&key,
+               Anum_pg_replication_origin_roname,
+               BTEqualStrategyNumber, F_TEXTEQ,
+               roname_d);
+   scan = systable_beginscan(rel, ReplicationOriginNameIndex,
+                             true /* indexOK */,
+                             SnapshotSelf,
+                             1, &key);
+   collides = HeapTupleIsValid(systable_getnext(scan));
+   systable_endscan(scan);
+
+   if (collides)
+       ereport(ERROR,
+               (errcode(ERRCODE_DUPLICATE_OBJECT),
+                errmsg("replication origin \"%s\" already exists",
+                       originname)));
with:
    if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
        ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
                       errmsg("replication origin \"%s\" already
exists", originname));

This will make the code smaller and easy to read. Thoughts?

Thanks,
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-09T04:13:40Z

On Thu, May 28, 2026 at 8:18 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
>
>> 3)
>>
>> +
>> + /* Dump replication origins */
>> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
>> + dumpReplicationOrigins(conn);
>>
>> why the check is for PG17 specifically?
>>
>
> In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
>

Okay, please add a comment in code for this.

Please find a few comments on v007:

1)

+ node = (ReplOriginId) node_oid;
+ originname = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ if (strlen(originname) > MAX_RONAME_LEN)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("replication origin name is too long"),
+ errdetail("Replication origin names must be no longer than %d bytes.",
+    MAX_RONAME_LEN)));
+
+ roname_d = CStringGetTextDatum(originname);

CStringGetTextDatum is:
#define CStringGetTextDatum(s) PointerGetDatum(cstring_to_text(s))
---------

We are first converting text to C-string , then Cstring to text and
then getting Datum. Double allocation and double conversion. Can we
please try this instead and verify if it works:

text *origin_text = PG_GETARG_TEXT_PP(1);
originname = text_to_cstring(origin_text);
roname_d = PointerGetDatum(origin_text);

2)
+check_new_cluster_replication_origins(void)
+{
+ PGconn     *conn;
+ PGresult   *res;
+ int         norigins;
+
+ /* Quick return if there are no replication origins to migrate. */
+ if (old_cluster.nrepl_origins == 0)
+ return;

Don't we need a version check simialr to what we have introduced in
pg_dumpall for origins? (Similar to
check_new_cluster_replication_slots as well)

3)
Since we have introduced check_new_cluster_replication_origins, do we
even need below checks in binary_upgrade_create_replication_origin()?
In which conditions will they be hit?

+ /* Check for OID collision */
+    ....
+ if (collides)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists", node_oid)));
+
+ /* Check for name collision */
+   ....
+ if (collides)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists",
+ originname)));

4)
Now since check_new_cluster_subscription_configuration() is purely
about checking max-origin GUC configuration, I think the name is
misleading. IMO, we should merge it to
check_new_cluster_replication_origins(). See how
check_new_cluster_replication_slots() does it: checking both
new-cluster's count and the max-configuration for slots.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-09T04:17:51Z

On Mon, Jun 8, 2026 at 10:36 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
>
> Hi Ajin,
> I have reviewed the patch and here are some comments:
>
> 1. I want to clarify, what is the expected behaviour when we upgrade
> the postgres instance from version <= 16 to "HEAD + patch"?
> -   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
> sizeof(originname));
> -   replorigin_create(originname);
> +   if (!IsBinaryUpgrade)
> +   {
> +       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
> originname, sizeof(originname));
> +       replorigin_create(originname);
> +   }
> and
> +           /* Dump replication origins */
> +           if (server_version >= 170000 && binary_upgrade &&
> archDumpFormat == archNull)
> +               dumpReplicationOrigins(conn);
>
> When the postgres instance with subscription is updated from PG 16 (or
> less) to HEAD, the replication origin is created as we do not have "if
> (!IsBinaryUpgrade)" check in HEAD,
> whereas when the similar instance is upgraded to "HEAD + patch", no
> replication origin is present after the upgrade.
> Is this difference in the behaviour between HEAD and "HEAD + patch" expected?
>

Good catch Shlok. IMO, the behaviour should be same even if we upgrade
to HEAD+patch, it should not result in the loss of origins.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-09T11:32:00Z

On Mon, 8 Jun 2026 at 22:36, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 28 May 2026 at 08:18, Ajin Cherian <itsajin@gmail.com> wrote:
> >
> >
> >
> > On Fri, May 22, 2026 at 8:27 PM shveta malik <shveta.malik@gmail.com> wrote:
> >>
> >> On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >> >
> >> > On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
> >> > >
> >> > > Rebased the patch as it was no longer applying.
> >> > >
> >> > Hi Ajin,
> >> >
> >> > I have started reviewing the patch. Here is my comment for v6-0002 patch:
> >> >
> >> > Suppose we have a replication setup: publisher -> subscriber
> >> > and we are upgrading subscriber to subscriber_new.
> >> > And if initially 'subscriber_new' has a replication origin, upgrading
> >> > the cluster can error out.
> >> >
> >> > Example:
> >> > We set up a logical replication between publisher node and subscriber node.
> >> >
> >> > On subscriber node:
> >> > postgres=# SELECT * FROM pg_replication_origin;
> >> >  roident |  roname
> >> > ---------+----------
> >> >        1 | pg_16393
> >> > (1 row)
> >> >
> >> > And initially subscriber_new has a replication origin:
> >> > postgres=# select pg_replication_origin_create('myname');
> >> >  pg_replication_origin_create
> >> > ------------------------------
> >> >                             1
> >> > (1 row)
> >> >
> >> > postgres=# SELECT * FROM pg_replication_origin;
> >> >  roident | roname
> >> > ---------+--------
> >> >        1 | myname
> >> > (1 row)
> >> >
> >> > Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
> >> > node, we get an error:
> >> > ```
> >> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> >> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> >> > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
> >> > ERROR:  replication origin with ID 1 already exists
> >> > ```
> >> >
> >> > This error occurs in "Performing Upgrade" stage. Should we add a check
> >> > in the "Performing Consistency Checks" stage so that we don't need to
> >> > re-initdb the new cluster to perform the upgrade?
> >> > Maybe we can add a check similar to
> >> > check_new_cluster_replication_slots(), where pg_upgrade errors out if
> >> > the new cluster already contains replication origins. Thoughts?
> >>
> >> +1. I had the same thought while reviewing the patch today. We should
> >> have it unless there is a reason we have avoided it??
> >
> >
> > Fixed this. Added a new check for replication origins and if the new cluster has any existing replication origins, then the check will fail.
> >>
> >>
> >> Few trivial comments:
> >>
> >> 1)
> >>
> >> +#include "access/skey.h"
> >> +#include "catalog/indexing.h"
> >>
> >> pg_upgrade_support.c compiles without above.
> >>
> >
> > Removed.
> >
> >>
> >> 2)
> >> + Assert(!OidIsValid(rel->rd_rel->reltoastrelid));
> >>
> >> Is there a reason for this sanity check? I generally do not see a
> >> Null-Toast table sanity check after every table_open.
> >>
> >
> > Removed.
> >
> >>
> >> 3)
> >>
> >> +
> >> + /* Dump replication origins */
> >> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
> >> + dumpReplicationOrigins(conn);
> >>
> >> why the check is for PG17 specifically?
> >>
> >
> > In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
> >
> >
> >
> > On Mon, May 25, 2026 at 5:13 PM shveta malik <shveta.malik@gmail.com> wrote:
> >>
> >>
> >> One issue in 002:
> >>
> >> binary_upgrade_create_replication_origin() has this:
> >>
> >> + originname = PG_GETARG_NAME(1);
> >> +
> >> + roname_d = CStringGetTextDatum(NameStr(*originname));
> >> +
> >>
> >> We are getting origin-name (text) into Name-type which can not be more
> >> than 64 bytes. So if an origin has name more than 64, it will end up
> >> trimming the name post-upgrade.
> >>
> >> I tried this:
> >>
> >> Old-setup:
> >> postgres=# SELECT
> >> pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64');
> >>  pg_replication_origin_create
> >> ------------------------------
> >>                             1
> >> postgres=# select * from pg_replication_origin;
> >>  roident |                                roname
> >> ---------+----------------------------------------------------------------------
> >>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64
> >>
> >>
> >> Post-upgrade: name got trimmed to 64 length.
> >> -------------------------
> >> postgres=#  select * from pg_replication_origin;
> >>  roident |                             roname
> >> ---------+-----------------------------------------------------------------
> >>        1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li
> >>
> >> thanks
> >> Shveta
> >
> >
> > Fixed this. Now  binary_upgrade_create_replication_origin handles it similarly to the way pg_replication_origin_create handles the name of the origin.
> >
> > Here's an updated version v7 containing these fixes.
>
> Hi Ajin,
> I have reviewed the patch and here are some comments:
>
> 1. I want to clarify, what is the expected behaviour when we upgrade
> the postgres instance from version <= 16 to "HEAD + patch"?
> -   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
> sizeof(originname));
> -   replorigin_create(originname);
> +   if (!IsBinaryUpgrade)
> +   {
> +       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
> originname, sizeof(originname));
> +       replorigin_create(originname);
> +   }
> and
> +           /* Dump replication origins */
> +           if (server_version >= 170000 && binary_upgrade &&
> archDumpFormat == archNull)
> +               dumpReplicationOrigins(conn);
>
> When the postgres instance with subscription is updated from PG 16 (or
> less) to HEAD, the replication origin is created as we do not have "if
> (!IsBinaryUpgrade)" check in HEAD,
> whereas when the similar instance is upgraded to "HEAD + patch", no
> replication origin is present after the upgrade.
> Is this difference in the behaviour between HEAD and "HEAD + patch" expected?
>
> 2.a) Can we replace:
> +   /* Check for OID collision */
> +   ScanKeyInit(&key,
> +               Anum_pg_replication_origin_roident,
> +               BTEqualStrategyNumber, F_OIDEQ,
> +               ObjectIdGetDatum(node));
> +   scan = systable_beginscan(rel, ReplicationOriginIdentIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides = HeapTupleIsValid(systable_getnext(scan));
> +   systable_endscan(scan);
> +
> +   if (collides)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                errmsg("replication origin with ID %u already
> exists", node_oid)));
>
> with:
> if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(node)))
>         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
>                        errmsg("replication origin with ID %u already
> exists", node_oid));
>
> 2.b) Similarly, Can we replace:
> +   /* Check for name collision */
> +   ScanKeyInit(&key,
> +               Anum_pg_replication_origin_roname,
> +               BTEqualStrategyNumber, F_TEXTEQ,
> +               roname_d);
> +   scan = systable_beginscan(rel, ReplicationOriginNameIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides = HeapTupleIsValid(systable_getnext(scan));
> +   systable_endscan(scan);
> +
> +   if (collides)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                errmsg("replication origin \"%s\" already exists",
> +                       originname)));
> with:
>     if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
>         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
>                        errmsg("replication origin \"%s\" already
> exists", originname));
>
> This will make the code smaller and easy to read. Thoughts?
>
One more comment:

3. In logical-replication.sgml, we have documentation as:
"Commit timestamps and origin data are not preserved during the upgrade."
Now since the origin related data is preserved after the upgrade,
should we update the documentation here?

Thanks,
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-11T06:38:13Z

On Mon, Jun 8, 2026 at 10:36 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
>
> 1. I want to clarify, what is the expected behaviour when we upgrade
> the postgres instance from version <= 16 to "HEAD + patch"?
> -   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
> sizeof(originname));
> -   replorigin_create(originname);
> +   if (!IsBinaryUpgrade)
> +   {
> +       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
> originname, sizeof(originname));
> +       replorigin_create(originname);
> +   }
> and
> +           /* Dump replication origins */
> +           if (server_version >= 170000 && binary_upgrade &&
> archDumpFormat == archNull)
> +               dumpReplicationOrigins(conn);
>
> When the postgres instance with subscription is updated from PG 16 (or
> less) to HEAD, the replication origin is created as we do not have "if
> (!IsBinaryUpgrade)" check in HEAD,
> whereas when the similar instance is upgraded to "HEAD + patch", no
> replication origin is present after the upgrade.
> Is this difference in the behaviour between HEAD and "HEAD + patch" expected?
>

No, this difference is not expected.

The issue is that in patch 002 we intentionally avoid
replication-origin creation during CreateSubscription() by adding the
'!IsBinaryUpgrade' check. The idea is that origin creation should
instead happen through
'binary_upgrade_create_conflict_detection_slot()', where we can also
preserve the
original origin ID.

Origin-ID preservation is currently implemented only for upgrades from
PG17 and later. (see the check in pg_dumpall.c in 002). As a result,
when upgrading from PG16, origin creation is skipped in
CreateSubscription(), but the origin is also not created through
binary_upgrade_create_conflict_detection_slot() because of the PG17
version check. Therefore, no replication origin exists after the
upgrade in this particular case.

We do not have access to the source-node version during the restore
stage (i.e., when CreateSubscription() is executed), so we cannot
perform the version check directly there. Therefore, any information
needed to make this decision must be preserved during the dump stage.

Ajin, Kuroda-san, and I discussed several possible approaches to address this:

1)
Add a new subscription option, for example 'skip_origin_creation'.
When set to true, CREATE SUBSCRIPTION would skip replication-origin
creation. During binary upgrade, pg_dump would emit
skip_origin_creation = true for source clusters running PG17 or later,
and false otherwise.

However, this approach requires introducing a new user-visible CREATE
SUBSCRIPTION option.

2)
Another idea could be to have a
'binary_upgrade_skip_subs_origin_creation' variable. We populate it
during dumpSubscription if 'source_version >= PG17' and
'IsBinaryUpgrade' is true. So this will be dumped

SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(true);
CREATE SUBSCRIPTION ...
SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(false);

First call will set 'binary_upgrade_skip_subs_origin_creation' which
will be used by Create Subscription to decide to skip origin. This is
similar to how dumpACl decides whether to record priveleges into
'pg_init_privs' for objectes created by extensions. See
binary_upgrade_set_record_init_privs() which sets
binary_upgrade_record_init_privs which is then used by
recordExtensionInitPriv().

3)
Create origins and preserve origin IDs regardless of source version

The current patch preserves origin IDs for upgrades from PG17 and later.

One reason for choosing PG17 as the starting point for preserving
origin-ids is that complete logical-replication setup migration was
introduced in PG17. Prior to PG17, subscription state
(pg_subscription_rel) and replication slots were not preserved during
upgrade, so limiting origin-ID preservation to PG17 and later aligns
with the broader logical-replication migration support.

OTOH, replication-origin migration itself is supported for PG16
upgrades. For PG16- upgrades (without this patch), subscriptions
itself can be migrated, and corresponding origins can be created at
that time. Therefore, even on versions prior to Pg17, it may also be
reasonable to consider preserving origin IDs independently of the rest
of the logical-replication configuration migration, and not tie it to
the PG17 boundary.

If we choose that approach, check_new_cluster_replication_origins()
would likely need to run for upgrades from PG16 (and prior) as well.
Subscription-ids also need to be preserved for the same versions. For
reference, check_new_cluster_replication_slots() and
check_new_cluster_subscription_configuration() currently run only for
PG17 and later.
~~

Given the need for a new user-visible parameter, I would rule out
option 1. Between options 2 and 3, I prefer option 2, but I would be
interested in hearing others' opinions. Any other ideas are also
welcome.

thanks
Shveta

RE: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-06-11T06:52:07Z

Dear Shveta,

> No, this difference is not expected.

Yes it must be fixed.

> 2)
> Another idea could be to have a
> 'binary_upgrade_skip_subs_origin_creation' variable. We populate it
> during dumpSubscription if 'source_version >= PG17' and
> 'IsBinaryUpgrade' is true. So this will be dumped
> 
> SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(true);
> CREATE SUBSCRIPTION ...
> SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(false);
> 
> First call will set 'binary_upgrade_skip_subs_origin_creation' which
> will be used by Create Subscription to decide to skip origin. This is
> similar to how dumpACl decides whether to record priveleges into
> 'pg_init_privs' for objectes created by extensions. See
> binary_upgrade_set_record_init_privs() which sets
> binary_upgrade_record_init_privs which is then used by
> recordExtensionInitPriv().

To clarify, I prefer the 2nd idea. According to 29d0a77, which migrates
replication slots from PG17+, it does not change the behavior when the source is
PG16-. We should follow the manner.

In case of third idea, origin id can be also preserved for PG16-, but the
version was out-of-scope of our patch [1]; in that version pg_subscription_rel
cannot be migrated, thus the logical replication cannot work in the first place.

Also, if we want to keep the existing behavior, we should allow upgrading the
below case.

Source: PG16, two subscriptions and origins exist (id = 1, 2).
Target: an origin exists (id = 1)

But we must reject upgrading if we preserves the origin-id, right?

[1]: https://www.postgresql.org/message-id/CAFPTHDYqpuZ6o2-HuCJDYqJ7GY3%2BzV%2BXo-gT7PAgi4Bkz%2BoTxw%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-11T10:37:44Z

On Thu, Jun 11, 2026 at 12:22 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shveta,
>
> > No, this difference is not expected.
>
> Yes it must be fixed.
>
> > 2)
> > Another idea could be to have a
> > 'binary_upgrade_skip_subs_origin_creation' variable. We populate it
> > during dumpSubscription if 'source_version >= PG17' and
> > 'IsBinaryUpgrade' is true. So this will be dumped
> >
> > SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(true);
> > CREATE SUBSCRIPTION ...
> > SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(false);
> >
> > First call will set 'binary_upgrade_skip_subs_origin_creation' which
> > will be used by Create Subscription to decide to skip origin. This is
> > similar to how dumpACl decides whether to record priveleges into
> > 'pg_init_privs' for objectes created by extensions. See
> > binary_upgrade_set_record_init_privs() which sets
> > binary_upgrade_record_init_privs which is then used by
> > recordExtensionInitPriv().
>
> To clarify, I prefer the 2nd idea. According to 29d0a77, which migrates
> replication slots from PG17+, it does not change the behavior when the source is
> PG16-. We should follow the manner.
>
> In case of third idea, origin id can be also preserved for PG16-, but the
> version was out-of-scope of our patch [1]; in that version pg_subscription_rel
> cannot be migrated, thus the logical replication cannot work in the first place.

I agree.

> Also, if we want to keep the existing behavior, we should allow upgrading the
> below case.
>
> Source: PG16, two subscriptions and origins exist (id = 1, 2).
> Target: an origin exists (id = 1)
>
> But we must reject upgrading if we preserves the origin-id, right?

Right. That is why I commented that if we choose option3, we will need
 to implement the check (check_new_cluster_replication_origins) even
on PG16 to reject the upgrade in the case you suggested.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Amit Kapila <amit.kapila16@gmail.com> — 2026-06-15T11:31:21Z

On Thu, Jun 11, 2026 at 12:08 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> Ajin, Kuroda-san, and I discussed several possible approaches to address this:
>
> 1)
> Add a new subscription option, for example 'skip_origin_creation'.
> When set to true, CREATE SUBSCRIPTION would skip replication-origin
> creation. During binary upgrade, pg_dump would emit
> skip_origin_creation = true for source clusters running PG17 or later,
> and false otherwise.
>
> However, this approach requires introducing a new user-visible CREATE
> SUBSCRIPTION option.
>
> 2)
> Another idea could be to have a
> 'binary_upgrade_skip_subs_origin_creation' variable. We populate it
> during dumpSubscription if 'source_version >= PG17' and
> 'IsBinaryUpgrade' is true. So this will be dumped
>
> SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(true);
> CREATE SUBSCRIPTION ...
> SELECT pg_catalog.binary_upgrade_set_skip_subs_origin_creation(false);
>
> First call will set 'binary_upgrade_skip_subs_origin_creation' which
> will be used by Create Subscription to decide to skip origin. This is
> similar to how dumpACl decides whether to record priveleges into
> 'pg_init_privs' for objectes created by extensions. See
> binary_upgrade_set_record_init_privs() which sets
> binary_upgrade_record_init_privs which is then used by
> recordExtensionInitPriv().
>
> 3)
> Create origins and preserve origin IDs regardless of source version
>
> The current patch preserves origin IDs for upgrades from PG17 and later.
>
> One reason for choosing PG17 as the starting point for preserving
> origin-ids is that complete logical-replication setup migration was
> introduced in PG17. Prior to PG17, subscription state
> (pg_subscription_rel) and replication slots were not preserved during
> upgrade, so limiting origin-ID preservation to PG17 and later aligns
> with the broader logical-replication migration support.
>
> OTOH, replication-origin migration itself is supported for PG16
> upgrades. For PG16- upgrades (without this patch), subscriptions
> itself can be migrated, and corresponding origins can be created at
> that time. Therefore, even on versions prior to Pg17, it may also be
> reasonable to consider preserving origin IDs independently of the rest
> of the logical-replication configuration migration, and not tie it to
> the PG17 boundary.
>
> If we choose that approach, check_new_cluster_replication_origins()
> would likely need to run for upgrades from PG16 (and prior) as well.
> Subscription-ids also need to be preserved for the same versions. For
> reference, check_new_cluster_replication_slots() and
> check_new_cluster_subscription_configuration() currently run only for
> PG17 and later.
> ~~
>
> Given the need for a new user-visible parameter, I would rule out
> option 1. Between options 2 and 3, I prefer option 2, but I would be
> interested in hearing others' opinions.
>

I'd prefer option-3 (preserve origin IDs uniformly, including upgrades
from PG16), for two reasons. (a) First, the new-cluster check ("reject
if the new cluster already has replication origins") is a restriction
on the target, and adding restrictions on a newer target version
regardless of source version is well within how pg_upgrade already
behaves. In practice the new cluster is a fresh initdb with no
origins, so the check effectively never fires; the only scenario it
rejects is one where the user manually created subscriptions/origins
on the new cluster before upgrading, which we discourage for other
object types anyway. So I don't think preserving existing behavior
here is worth much. (b) Second, option-2 reintroduces source-version
branching at dump time (the skip_subs_origin_creation flag gated on
source >= 17). Since pg_commit_ts is only copied from PG19+, the exact
roident value only matters from PG19; for PG16-18 the value is
don't-care, so preserving it everywhere is harmless where it isn't
needed and required where it is. That lets us keep a single
always-preserve path instead of branching per source version.

Separately, I don't think we actually need to preserve the
subscription OID to meet the goal here. pg_commit_ts stores the
numeric roident, not the origin name or the sub OID, and conflict
detection compares roidents. So the invariant we must keep is "each
subscription owns the same roident after upgrade", which doesn't
depend on the name being stable.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-06-16T09:20:01Z

On Mon, Jun 15, 2026 at 9:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

>
>
> I'd prefer option-3 (preserve origin IDs uniformly, including upgrades
> from PG16), for two reasons. (a) First, the new-cluster check ("reject
> if the new cluster already has replication origins") is a restriction
> on the target, and adding restrictions on a newer target version
> regardless of source version is well within how pg_upgrade already
> behaves. In practice the new cluster is a fresh initdb with no
> origins, so the check effectively never fires; the only scenario it
> rejects is one where the user manually created subscriptions/origins
> on the new cluster before upgrading, which we discourage for other
> object types anyway. So I don't think preserving existing behavior
> here is worth much. (b) Second, option-2 reintroduces source-version
> branching at dump time (the skip_subs_origin_creation flag gated on
> source >= 17). Since pg_commit_ts is only copied from PG19+, the exact
> roident value only matters from PG19; for PG16-18 the value is
> don't-care, so preserving it everywhere is harmless where it isn't
> needed and required where it is. That lets us keep a single
> always-preserve path instead of branching per source version.
>
>
Ok.

> Separately, I don't think we actually need to preserve the
> subscription OID to meet the goal here. pg_commit_ts stores the
> numeric roident, not the origin name or the sub OID, and conflict
> detection compares roidents. So the invariant we must keep is "each
> subscription owns the same roident after upgrade", which doesn't
> depend on the name being stable.
>
>
>
Replication origins are global objects and are therefore dumped by
pg_dumpall, while subscriptions are database-specific and are dumped by
pg_dump. In pg_upgrade, pg_dumpall runs before pg_dump.

The complication if subscription OID is not preserved is that
subscription-associated origins derive their name from the subscription's
OID. If the subscription OID is not preserved across upgrades, the origin
name on the new cluster will differ from the old one, making it impossible
to restore the origin independently. For these origins, creation must
happen after CreateSubscription has established the new OID and not upfront
in pg_dumpall.

Non-subscription origins have no such dependency and continue to be created
separately as before. The subscription code also cannot be left untouched:
even once the subscription is created, rather than origin being created
inside CreateSubscription, its associated origin must still be explicitly
created via binary_upgrade_create_replication_origin to ensure the roident
matches the original node, which means CreateSubscription needs to be
modified anyways. So, there is an advantage of reducing code if
subscription OID is preserved and all origins keep the same name as the old
node.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-06-17T09:19:19Z

On Fri, Jun 5, 2026 at 8:10 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Few comments:
> 1) pg_dumpall dumps all replication origins, not just subscription
> origins and recreates every origin:
> SELECT o.*, os.remote_lsn
> FROM pg_replication_origin o
> LEFT JOIN pg_replication_origin_status os
>   ON o.roident = os.local_id
>
> Is pg_upgrade expected to preserve user-created origins?
> If so, you can mention this in the commit message
>

I explicitly state that all replication origins are migrated in the
commit message, nothing specific about subscription origins.

> 2) Should the table lock be released here, if so should we mention
> comments similar to how it is mentioned in
> binary_upgarde_add_sub_rel_state:
> + /* Restore the remote_lsn if provided, while still holding the lock */
> + if (!PG_ARGISNULL(2))
> + {
> + XLogRecPtr remote_commit = PG_GETARG_LSN(2);
>
> - ReplicationSlotRelease();
> + replorigin_advance(node, remote_commit, InvalidXLogRecPtr,
> +    false /* backward */,
> +    false /* WAL log */);
> + }
> +
> + table_close(rel, RowExclusiveLock);
>

Is that relevant when creating a new row? or only when updating an existing one?

> 3) We can use new style of ereport to exclude '(' before errcode:
> 3.a)
> + ereport(ERROR,
> + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> + errmsg("replication origin name is too long"),
> + errdetail("Replication origin names must be no longer than %d bytes.",
> +    MAX_RONAME_LEN)));
>
> 3.b) Similarly here:
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin with ID %u already exists", node_oid)));
>
> 3.c) There are many others, you can search other ereport in the patch
> and change it too.
>

Done.

> 4) Since only one column will be returned here, can we use
> "PQgetvalue(res, 0, 0) and remove PQfnumber:
> + res = executeQueryOrDie(conn,
> + "SELECT count(*) AS nrepl_origins "
> + "FROM pg_catalog.pg_replication_origin");
> + i_nrepl_origins = PQfnumber(res, "nrepl_origins");
> + cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_nrepl_origins));
> + PQclear(res);
>

Done.

> 5) replorigin_create also has some duplicate code like below, will it
> be possible to have a common function so that both of them can use?
> /* Check for name collision */
> + ScanKeyInit(&key,
> + Anum_pg_replication_origin_roname,
> + BTEqualStrategyNumber, F_TEXTEQ,
> + roname_d);
> + scan = systable_beginscan(rel, ReplicationOriginNameIndex,
> +   true /* indexOK */,
> +   SnapshotSelf,
> +   1, &key);
> + collides = HeapTupleIsValid(systable_getnext(scan));
> + systable_endscan(scan);
> +
> + if (collides)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin \"%s\" already exists",
> + originname)));
> +
> + memset(&nulls, 0, sizeof(nulls));
> + memset(&values, 0, sizeof(values));
> +
> + values[Anum_pg_replication_origin_roident - 1] = ObjectIdGetDatum(node);
> + values[Anum_pg_replication_origin_roname - 1] = roname_d;
> +
> + tuple = heap_form_tuple(RelationGetDescr(rel), values, nulls);
> + CatalogTupleInsert(rel, tuple);
> + heap_freetuple(tuple);
> + CommandCounterIncrement();
>

I have created a new function replorigin_create_with_id() which has
some common code and called that from both replorigin_create() and
binary_upgrade_create_replication_origin()


On Tue, Jun 9, 2026 at 3:06 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Hi Ajin,
> I have reviewed the patch and here are some comments:
>
> 1. I want to clarify, what is the expected behaviour when we upgrade
> the postgres instance from version <= 16 to "HEAD + patch"?
> -   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
> sizeof(originname));
> -   replorigin_create(originname);
> +   if (!IsBinaryUpgrade)
> +   {
> +       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
> originname, sizeof(originname));
> +       replorigin_create(originname);
> +   }
> and
> +           /* Dump replication origins */
> +           if (server_version >= 170000 && binary_upgrade &&
> archDumpFormat == archNull)
> +               dumpReplicationOrigins(conn);
>
> When the postgres instance with subscription is updated from PG 16 (or
> less) to HEAD, the replication origin is created as we do not have "if
> (!IsBinaryUpgrade)" check in HEAD,
> whereas when the similar instance is upgraded to "HEAD + patch", no
> replication origin is present after the upgrade.
> Is this difference in the behaviour between HEAD and "HEAD + patch" expected?
>

I have modified this as option 3 suggested by Shveta and Amit and
removed the PG17 restriction for migrating replication origins, now it
does it for all versions that have replication origins.

> 2.a) Can we replace:
> +   /* Check for OID collision */
> +   ScanKeyInit(&key,
> +               Anum_pg_replication_origin_roident,
> +               BTEqualStrategyNumber, F_OIDEQ,
> +               ObjectIdGetDatum(node));
> +   scan = systable_beginscan(rel, ReplicationOriginIdentIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides = HeapTupleIsValid(systable_getnext(scan));
> +   systable_endscan(scan);
> +
> +   if (collides)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                errmsg("replication origin with ID %u already
> exists", node_oid)));
>
> with:
> if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(node)))
>         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
>                        errmsg("replication origin with ID %u already
> exists", node_oid));
>
> 2.b) Similarly, Can we replace:
> +   /* Check for name collision */
> +   ScanKeyInit(&key,
> +               Anum_pg_replication_origin_roname,
> +               BTEqualStrategyNumber, F_TEXTEQ,
> +               roname_d);
> +   scan = systable_beginscan(rel, ReplicationOriginNameIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides = HeapTupleIsValid(systable_getnext(scan));
> +   systable_endscan(scan);
> +
> +   if (collides)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                errmsg("replication origin \"%s\" already exists",
> +                       originname)));
> with:
>     if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
>         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
>                        errmsg("replication origin \"%s\" already
> exists", originname));
>
> This will make the code smaller and easy to read. Thoughts?

Done


On Tue, Jun 9, 2026 at 2:13 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Please find a few comments on v007:
>
> 1)
>
> + node = (ReplOriginId) node_oid;
> + originname = text_to_cstring(PG_GETARG_TEXT_PP(1));
> +
> + if (strlen(originname) > MAX_RONAME_LEN)
> + ereport(ERROR,
> + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> + errmsg("replication origin name is too long"),
> + errdetail("Replication origin names must be no longer than %d bytes.",
> +    MAX_RONAME_LEN)));
> +
> + roname_d = CStringGetTextDatum(originname);
>
> CStringGetTextDatum is:
> #define CStringGetTextDatum(s) PointerGetDatum(cstring_to_text(s))
> ---------
>
> We are first converting text to C-string , then Cstring to text and
> then getting Datum. Double allocation and double conversion. Can we
> please try this instead and verify if it works:
>
> text *origin_text = PG_GETARG_TEXT_PP(1);
> originname = text_to_cstring(origin_text);
> roname_d = PointerGetDatum(origin_text);
>

I've restructured the code now, its a bit tough to do this now.

> 2)
> +check_new_cluster_replication_origins(void)
> +{
> + PGconn     *conn;
> + PGresult   *res;
> + int         norigins;
> +
> + /* Quick return if there are no replication origins to migrate. */
> + if (old_cluster.nrepl_origins == 0)
> + return;
>
> Don't we need a version check simialr to what we have introduced in
> pg_dumpall for origins? (Similar to
> check_new_cluster_replication_slots as well)
>

I've removed the version check for PG17 now.

> 3)
> Since we have introduced check_new_cluster_replication_origins, do we
> even need below checks in binary_upgrade_create_replication_origin()?
> In which conditions will they be hit?
>
> + /* Check for OID collision */
> +    ....
> + if (collides)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin with ID %u already exists", node_oid)));
> +
> + /* Check for name collision */
> +   ....
> + if (collides)
> + ereport(ERROR,
> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin \"%s\" already exists",
> + originname)));
>

With the restructured code, its not easy to do this now.

>
> 4)
> Now since check_new_cluster_subscription_configuration() is purely
> about checking max-origin GUC configuration, I think the name is
> misleading. IMO, we should merge it to
> check_new_cluster_replication_origins(). See how
> check_new_cluster_replication_slots() does it: checking both
> new-cluster's count and the max-configuration for slots.
>

Done.


On Tue, Jun 9, 2026 at 9:32 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> >
> One more comment:
>
> 3. In logical-replication.sgml, we have documentation as:
> "Commit timestamps and origin data are not preserved during the upgrade."
> Now since the origin related data is preserved after the upgrade,
> should we update the documentation here?
>

Updated.

I have addressed the comments in v8 (attached). I've also now taken
out the check for PG17 on old cluster and now all clusters which have
replication origins are migrated.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Nisha Moond <nisha.moond412@gmail.com> — 2026-06-17T10:59:11Z

On Wed, Jun 17, 2026 at 2:49 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> I have addressed the comments in v8 (attached). I've also now taken
> out the check for PG17 on old cluster and now all clusters which have
> replication origins are migrated.

Hi Ajin, Thanks for the patches.
I have started reviewing the patches. Please find a few initial
comments below (from v7 review, still applicable to v8):

1) Patch-002: pg_dump.c
+ if (dopt->binary_upgrade && subinfo->subenabled &&
fout->remoteVersion >= 170000)
  {
- if (subinfo->suboriginremotelsn)
- {
- /*
- * Preserve the remote_lsn for the subscriber's replication

I could not find any usage of subinfo[i].suboriginremotelsn anymore.
It seems to be dead code now. Can we remove it?
~~~

2) Patch-002 : binary_upgrade_create_replication_origin()
- PG_RETURN_VOID();
+ node_oid = PG_GETARG_OID(0);
+
+ if (node_oid == InvalidOid || node_oid > PG_UINT16_MAX)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("replication origin ID %u is out of range", node_oid));
+

The above range check allows PG_UINT16_MAX, but that value is not part
of the valid replication origin ID range.

In origin.h:
  #define DoNotReplicateId PG_UINT16_MAX

And we already ensure "ReplOriginId != DoNotReplicateId" everywhere
else. Would something like the below be more appropriate?
  if (node_oid == InvalidReplOriginId || node_oid >= DoNotReplicateId)
    ereport(ERROR, ...
~~~

3) I think we should add some additional details in
logical-replication.sgml, especially under <title>Prepare for
Subscriber Upgrades</title>.
Some suggestions:
3a) Since all replication origins are now migrated, this:
  "...greater than or equal to the number of subscriptions present in
the old cluster."
could become:
  "...greater than or equal to the number of replication origins
present in the old cluster."

3b) We should also mention:
  The new cluster must contain no replication origins.

--
Thanks,
Nisha

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Nisha Moond <nisha.moond412@gmail.com> — 2026-06-17T11:42:22Z

On Wed, Jun 17, 2026 at 4:29 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Jun 17, 2026 at 2:49 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > I have addressed the comments in v8 (attached). I've also now taken
> > out the check for PG17 on old cluster and now all clusters which have
> > replication origins are migrated.
>
> Hi Ajin, Thanks for the patches.
> I have started reviewing the patches. Please find a few initial
> comments below (from v7 review, still applicable to v8):

Please find a couple more comments for the v8-002 patch:

4) I was able to build successfully without all three new header
inclusions below, so we may not need them.
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -11,6 +11,7 @@
+#include "access/genam.h"
...
+#include "utils/fmgroids.h"
...
+#include "utils/snapmgr.h"
~~~

5) Race condition in origin.c: replorigin_create()
- heap_freetuple(tuple);
+ table_close(rel, ExclusiveLock);
+
+ replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
+

Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
of a race between two concurrent CREATE SUBSCRIPTION commands:

postgres=# create subscription sub2 connection 'dbname=postgres
host=localhost port=7733' publication pub1;
ERROR:  replication origin with ID 2 already exists

I think this happens because the table lock is released, allowing
another session to create a pg_origin with the next available roident,
which replorigin_create_with_id() later attempts to use.

Would it make sense to call replorigin_create_with_id() before
table_close() so the lock is still held? I tested this and it works,
though I'm not fully sure whether self-locking could be a concern
here, since replorigin_create_with_id() again acquires a
RowExclusiveLock on the same table.
~~~

--
Thanks,
Nisha

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-18T09:16:51Z

On Wed, Jun 17, 2026 at 5:12 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Wed, Jun 17, 2026 at 4:29 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Wed, Jun 17, 2026 at 2:49 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > I have addressed the comments in v8 (attached). I've also now taken
> > > out the check for PG17 on old cluster and now all clusters which have
> > > replication origins are migrated.
> >
> > Hi Ajin, Thanks for the patches.
> > I have started reviewing the patches. Please find a few initial
> > comments below (from v7 review, still applicable to v8):
>
> Please find a couple more comments for the v8-002 patch:
>
> 4) I was able to build successfully without all three new header
> inclusions below, so we may not need them.
> --- a/src/backend/utils/adt/pg_upgrade_support.c
> +++ b/src/backend/utils/adt/pg_upgrade_support.c
> @@ -11,6 +11,7 @@
> +#include "access/genam.h"
> ...
> +#include "utils/fmgroids.h"
> ...
> +#include "utils/snapmgr.h"
> ~~~
>
> 5) Race condition in origin.c: replorigin_create()
> - heap_freetuple(tuple);
> + table_close(rel, ExclusiveLock);
> +
> + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
> +
>
> Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
> of a race between two concurrent CREATE SUBSCRIPTION commands:
>
> postgres=# create subscription sub2 connection 'dbname=postgres
> host=localhost port=7733' publication pub1;
> ERROR:  replication origin with ID 2 already exists
>
> I think this happens because the table lock is released, allowing
> another session to create a pg_origin with the next available roident,
> which replorigin_create_with_id() later attempts to use.
>
> Would it make sense to call replorigin_create_with_id() before
> table_close() so the lock is still held? I tested this and it works,
> though I'm not fully sure whether self-locking could be a concern
> here, since replorigin_create_with_id() again acquires a
> RowExclusiveLock on the same table.

We can handle this race-condition by moving the table-open to callers
rather than internal call replorigin_create_with_id().  But we also
need the following changes first:

1)
replorigin_create_with_id has:

+ /*
+ * To avoid needing a TOAST table for pg_replication_origin, we limit
+ * replication origin names to 512 bytes.  This should be more than enough
+ * for all practical use.
+ */
+ if (strlen(roname) > MAX_RONAME_LEN)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("replication origin name is too long"),
+ errdetail("Replication origin names must be no longer than %d bytes.",
+   MAX_RONAME_LEN));

replorigin_create_with_id() is called by replorigin_create() and
binary_upgrade_create_replication_origin().   replorigin_create()
already has this check. We should move above check to
binary_upgrade_create_replication_origin() and get rid of it from
internal function replorigin_create_with_id().

2)

Also the checks:
+ if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(roident)))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists",
+    (Oid) roident));
+
+ if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists", roname));

should be moved to caller binary_upgrade_create_replication_origin().
IMO, these checks are not needed for replorigin_create(), it alread
has this, which should suffice.

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

~~

Once we correctly place these checks, we can move the 'table_open with
lock' to caller (before name and id collision checks) instead of
replorigin_create_with_id(), which will take care of race-conditions
reported by Nisha.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-19T03:59:47Z

On Tue, Jun 16, 2026 at 2:50 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
>
>
> On Mon, Jun 15, 2026 at 9:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>>
>>
>> I'd prefer option-3 (preserve origin IDs uniformly, including upgrades
>> from PG16), for two reasons. (a) First, the new-cluster check ("reject
>> if the new cluster already has replication origins") is a restriction
>> on the target, and adding restrictions on a newer target version
>> regardless of source version is well within how pg_upgrade already
>> behaves. In practice the new cluster is a fresh initdb with no
>> origins, so the check effectively never fires; the only scenario it
>> rejects is one where the user manually created subscriptions/origins
>> on the new cluster before upgrading, which we discourage for other
>> object types anyway. So I don't think preserving existing behavior
>> here is worth much. (b) Second, option-2 reintroduces source-version
>> branching at dump time (the skip_subs_origin_creation flag gated on
>> source >= 17). Since pg_commit_ts is only copied from PG19+, the exact
>> roident value only matters from PG19; for PG16-18 the value is
>> don't-care, so preserving it everywhere is harmless where it isn't
>> needed and required where it is. That lets us keep a single
>> always-preserve path instead of branching per source version.
>>
>
> Ok.
>
>>
>> Separately, I don't think we actually need to preserve the
>> subscription OID to meet the goal here. pg_commit_ts stores the
>> numeric roident, not the origin name or the sub OID, and conflict
>> detection compares roidents. So the invariant we must keep is "each
>> subscription owns the same roident after upgrade", which doesn't
>> depend on the name being stable.
>>
>>
>
> Replication origins are global objects and are therefore dumped by pg_dumpall, while subscriptions are database-specific and are dumped by pg_dump. In pg_upgrade, pg_dumpall runs before pg_dump.
>
> The complication if subscription OID is not preserved is that subscription-associated origins derive their name from the subscription's OID. If the subscription OID is not preserved across upgrades, the origin name on the new cluster will differ from the old one, making it impossible to restore the origin independently. For these origins, creation must happen after CreateSubscription has established the new OID and not upfront in pg_dumpall.
>
> Non-subscription origins have no such dependency and continue to be created separately as before. The subscription code also cannot be left untouched: even once the subscription is created, rather than origin being created inside CreateSubscription, its associated origin must still be explicitly created via binary_upgrade_create_replication_origin to ensure the roident matches the original node, which means CreateSubscription needs to be modified anyways. So, there is an advantage of reducing code if subscription OID is preserved and all origins keep the same name as the old node.
>

IIUC, the conclusion (and what's implemented in v8) is:

1) The origin-ids are preserved on upgraded version irrespective of
source version.
2) Since origin-ids preservation requires sub-oid preservation due to
the origin-name's dependency on the sub-oid, we preserve sub-oid too
irrespective of source-version.

Let's see what Amit has to say about this.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-19T07:48:34Z

On Wed, 17 Jun 2026 at 14:49, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Jun 5, 2026 at 8:10 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Few comments:
> > 1) pg_dumpall dumps all replication origins, not just subscription
> > origins and recreates every origin:
> > SELECT o.*, os.remote_lsn
> > FROM pg_replication_origin o
> > LEFT JOIN pg_replication_origin_status os
> >   ON o.roident = os.local_id
> >
> > Is pg_upgrade expected to preserve user-created origins?
> > If so, you can mention this in the commit message
> >
>
> I explicitly state that all replication origins are migrated in the
> commit message, nothing specific about subscription origins.
>
> > 2) Should the table lock be released here, if so should we mention
> > comments similar to how it is mentioned in
> > binary_upgarde_add_sub_rel_state:
> > + /* Restore the remote_lsn if provided, while still holding the lock */
> > + if (!PG_ARGISNULL(2))
> > + {
> > + XLogRecPtr remote_commit = PG_GETARG_LSN(2);
> >
> > - ReplicationSlotRelease();
> > + replorigin_advance(node, remote_commit, InvalidXLogRecPtr,
> > +    false /* backward */,
> > +    false /* WAL log */);
> > + }
> > +
> > + table_close(rel, RowExclusiveLock);
> >
>
> Is that relevant when creating a new row? or only when updating an existing one?
>
> > 3) We can use new style of ereport to exclude '(' before errcode:
> > 3.a)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> > + errmsg("replication origin name is too long"),
> > + errdetail("Replication origin names must be no longer than %d bytes.",
> > +    MAX_RONAME_LEN)));
> >
> > 3.b) Similarly here:
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists", node_oid)));
> >
> > 3.c) There are many others, you can search other ereport in the patch
> > and change it too.
> >
>
> Done.
>
> > 4) Since only one column will be returned here, can we use
> > "PQgetvalue(res, 0, 0) and remove PQfnumber:
> > + res = executeQueryOrDie(conn,
> > + "SELECT count(*) AS nrepl_origins "
> > + "FROM pg_catalog.pg_replication_origin");
> > + i_nrepl_origins = PQfnumber(res, "nrepl_origins");
> > + cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_nrepl_origins));
> > + PQclear(res);
> >
>
> Done.
>
> > 5) replorigin_create also has some duplicate code like below, will it
> > be possible to have a common function so that both of them can use?
> > /* Check for name collision */
> > + ScanKeyInit(&key,
> > + Anum_pg_replication_origin_roname,
> > + BTEqualStrategyNumber, F_TEXTEQ,
> > + roname_d);
> > + scan = systable_beginscan(rel, ReplicationOriginNameIndex,
> > +   true /* indexOK */,
> > +   SnapshotSelf,
> > +   1, &key);
> > + collides = HeapTupleIsValid(systable_getnext(scan));
> > + systable_endscan(scan);
> > +
> > + if (collides)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin \"%s\" already exists",
> > + originname)));
> > +
> > + memset(&nulls, 0, sizeof(nulls));
> > + memset(&values, 0, sizeof(values));
> > +
> > + values[Anum_pg_replication_origin_roident - 1] = ObjectIdGetDatum(node);
> > + values[Anum_pg_replication_origin_roname - 1] = roname_d;
> > +
> > + tuple = heap_form_tuple(RelationGetDescr(rel), values, nulls);
> > + CatalogTupleInsert(rel, tuple);
> > + heap_freetuple(tuple);
> > + CommandCounterIncrement();
> >
>
> I have created a new function replorigin_create_with_id() which has
> some common code and called that from both replorigin_create() and
> binary_upgrade_create_replication_origin()
>
>
> On Tue, Jun 9, 2026 at 3:06 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > Hi Ajin,
> > I have reviewed the patch and here are some comments:
> >
> > 1. I want to clarify, what is the expected behaviour when we upgrade
> > the postgres instance from version <= 16 to "HEAD + patch"?
> > -   ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
> > sizeof(originname));
> > -   replorigin_create(originname);
> > +   if (!IsBinaryUpgrade)
> > +   {
> > +       ReplicationOriginNameForLogicalRep(subid, InvalidOid,
> > originname, sizeof(originname));
> > +       replorigin_create(originname);
> > +   }
> > and
> > +           /* Dump replication origins */
> > +           if (server_version >= 170000 && binary_upgrade &&
> > archDumpFormat == archNull)
> > +               dumpReplicationOrigins(conn);
> >
> > When the postgres instance with subscription is updated from PG 16 (or
> > less) to HEAD, the replication origin is created as we do not have "if
> > (!IsBinaryUpgrade)" check in HEAD,
> > whereas when the similar instance is upgraded to "HEAD + patch", no
> > replication origin is present after the upgrade.
> > Is this difference in the behaviour between HEAD and "HEAD + patch" expected?
> >
>
> I have modified this as option 3 suggested by Shveta and Amit and
> removed the PG17 restriction for migrating replication origins, now it
> does it for all versions that have replication origins.
>
> > 2.a) Can we replace:
> > +   /* Check for OID collision */
> > +   ScanKeyInit(&key,
> > +               Anum_pg_replication_origin_roident,
> > +               BTEqualStrategyNumber, F_OIDEQ,
> > +               ObjectIdGetDatum(node));
> > +   scan = systable_beginscan(rel, ReplicationOriginIdentIndex,
> > +                             true /* indexOK */,
> > +                             SnapshotSelf,
> > +                             1, &key);
> > +   collides = HeapTupleIsValid(systable_getnext(scan));
> > +   systable_endscan(scan);
> > +
> > +   if (collides)
> > +       ereport(ERROR,
> > +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> > +                errmsg("replication origin with ID %u already
> > exists", node_oid)));
> >
> > with:
> > if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(node)))
> >         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
> >                        errmsg("replication origin with ID %u already
> > exists", node_oid));
> >
> > 2.b) Similarly, Can we replace:
> > +   /* Check for name collision */
> > +   ScanKeyInit(&key,
> > +               Anum_pg_replication_origin_roname,
> > +               BTEqualStrategyNumber, F_TEXTEQ,
> > +               roname_d);
> > +   scan = systable_beginscan(rel, ReplicationOriginNameIndex,
> > +                             true /* indexOK */,
> > +                             SnapshotSelf,
> > +                             1, &key);
> > +   collides = HeapTupleIsValid(systable_getnext(scan));
> > +   systable_endscan(scan);
> > +
> > +   if (collides)
> > +       ereport(ERROR,
> > +               (errcode(ERRCODE_DUPLICATE_OBJECT),
> > +                errmsg("replication origin \"%s\" already exists",
> > +                       originname)));
> > with:
> >     if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
> >         ereport(ERROR, errcode(ERRCODE_DUPLICATE_OBJECT),
> >                        errmsg("replication origin \"%s\" already
> > exists", originname));
> >
> > This will make the code smaller and easy to read. Thoughts?
>
> Done
>
>
> On Tue, Jun 9, 2026 at 2:13 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > Please find a few comments on v007:
> >
> > 1)
> >
> > + node = (ReplOriginId) node_oid;
> > + originname = text_to_cstring(PG_GETARG_TEXT_PP(1));
> > +
> > + if (strlen(originname) > MAX_RONAME_LEN)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> > + errmsg("replication origin name is too long"),
> > + errdetail("Replication origin names must be no longer than %d bytes.",
> > +    MAX_RONAME_LEN)));
> > +
> > + roname_d = CStringGetTextDatum(originname);
> >
> > CStringGetTextDatum is:
> > #define CStringGetTextDatum(s) PointerGetDatum(cstring_to_text(s))
> > ---------
> >
> > We are first converting text to C-string , then Cstring to text and
> > then getting Datum. Double allocation and double conversion. Can we
> > please try this instead and verify if it works:
> >
> > text *origin_text = PG_GETARG_TEXT_PP(1);
> > originname = text_to_cstring(origin_text);
> > roname_d = PointerGetDatum(origin_text);
> >
>
> I've restructured the code now, its a bit tough to do this now.
>
> > 2)
> > +check_new_cluster_replication_origins(void)
> > +{
> > + PGconn     *conn;
> > + PGresult   *res;
> > + int         norigins;
> > +
> > + /* Quick return if there are no replication origins to migrate. */
> > + if (old_cluster.nrepl_origins == 0)
> > + return;
> >
> > Don't we need a version check simialr to what we have introduced in
> > pg_dumpall for origins? (Similar to
> > check_new_cluster_replication_slots as well)
> >
>
> I've removed the version check for PG17 now.
>
> > 3)
> > Since we have introduced check_new_cluster_replication_origins, do we
> > even need below checks in binary_upgrade_create_replication_origin()?
> > In which conditions will they be hit?
> >
> > + /* Check for OID collision */
> > +    ....
> > + if (collides)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists", node_oid)));
> > +
> > + /* Check for name collision */
> > +   ....
> > + if (collides)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin \"%s\" already exists",
> > + originname)));
> >
>
> With the restructured code, its not easy to do this now.
>
> >
> > 4)
> > Now since check_new_cluster_subscription_configuration() is purely
> > about checking max-origin GUC configuration, I think the name is
> > misleading. IMO, we should merge it to
> > check_new_cluster_replication_origins(). See how
> > check_new_cluster_replication_slots() does it: checking both
> > new-cluster's count and the max-configuration for slots.
> >
>
> Done.
>
>
> On Tue, Jun 9, 2026 at 9:32 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > >
> > One more comment:
> >
> > 3. In logical-replication.sgml, we have documentation as:
> > "Commit timestamps and origin data are not preserved during the upgrade."
> > Now since the origin related data is preserved after the upgrade,
> > should we update the documentation here?
> >
>
> Updated.
>
> I have addressed the comments in v8 (attached). I've also now taken
> out the check for PG17 on old cluster and now all clusters which have
> replication origins are migrated.

Hi Ajin,

Thanks for providing the updated patch, I reviewed the patch and I
have some comments:

1. patch is not applying on HEAD and needs a rebase.

2. New line after 'check_new_cluster_replication_origins' is not required:
+ check_new_cluster_replication_origins();
+

3. pg_dumpall with option '--roles-only' also dumps replication origins
eg:
./pg_dumpall -p 9000 --binary-upgrade --roles-only > op.sql
output contains:
-- For binary upgrade, must preserve replication origin roident and remote_lsn
SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);

I think in this case we should not dump the replication origins as we
explicitly gave the option of '--roles-only'

4. pg_dumpall with --globals-only option also dumps replication origins.
eg:
./pg_dumpall -p 9000 --binary-upgrade --globals-only> op.sql and
output contains:
-- For binary upgrade, must preserve replication origin roident and remote_lsn
SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);

Is this behaviour intentional? Other logical replication objects
(e.g., publications and subscriptions) are not included in pg_dumpall
--globals-only, so is it okay to dump replication origin?

Thanks,
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Rui Zhao <zhaorui126@gmail.com> — 2026-06-21T16:22:14Z

>
> Hi Ajin,
>
> Thanks for v8. A couple of comments:
>
> 1) The PG17 gating seems inconsistent between the dump and the check now.
> dumpReplicationOrigins() is called unconditionally:
>
>     if (binary_upgrade && archDumpFormat == archNull)
>         dumpReplicationOrigins(conn);
>
> but get_subscription_info(), which sets nrepl_origins, is still only
> called for old clusters >= 17 in check_new_cluster():
>
>     if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
>         ...
>         get_subscription_info(&old_cluster);
>
> and check_new_cluster_replication_origins() returns early on it:
>
>     if (old_cluster.nrepl_origins == 0)
>         return;
>
> For a < 17 old cluster get_subscription_info() is never called, so
> nrepl_origins stays 0 and this returns early. So when such a cluster has a
> (user-created) origin, the origin still gets dumped and recreated, while
> the "new cluster must have no origins" and max_active_replication_origins
> checks are skipped, and the upgrade fails during restore instead of at the
> check step. nrepl_origins is just a count(*) on pg_replication_origin, so
> moving its collection out of the >= 1700 block should be enough.
>
> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Since
> the patch migrates all origins, should we also create a user origin
> (pg_replication_origin_create()) and verify its roident/remote_lsn after
> the upgrade? That would also cover the case in (1).
>
> Thanks,
> Rui
>

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-06-23T11:50:18Z

On Wed, Jun 17, 2026 at 8:59 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Hi Ajin, Thanks for the patches.
> I have started reviewing the patches. Please find a few initial
> comments below (from v7 review, still applicable to v8):
>
> 1) Patch-002: pg_dump.c
> + if (dopt->binary_upgrade && subinfo->subenabled &&
> fout->remoteVersion >= 170000)
>   {
> - if (subinfo->suboriginremotelsn)
> - {
> - /*
> - * Preserve the remote_lsn for the subscriber's replication
>
> I could not find any usage of subinfo[i].suboriginremotelsn anymore.
> It seems to be dead code now. Can we remove it?

Removed it.


> ~~~
>
> 2) Patch-002 : binary_upgrade_create_replication_origin()
> - PG_RETURN_VOID();
> + node_oid = PG_GETARG_OID(0);
> +
> + if (node_oid == InvalidOid || node_oid > PG_UINT16_MAX)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("replication origin ID %u is out of range", node_oid));
> +
>
> The above range check allows PG_UINT16_MAX, but that value is not part
> of the valid replication origin ID range.
>
> In origin.h:
>   #define DoNotReplicateId PG_UINT16_MAX
>
> And we already ensure "ReplOriginId != DoNotReplicateId" everywhere
> else. Would something like the below be more appropriate?
>   if (node_oid == InvalidReplOriginId || node_oid >= DoNotReplicateId)
>     ereport(ERROR, ...
> ~~~

Done.


>
> 3) I think we should add some additional details in
> logical-replication.sgml, especially under <title>Prepare for
> Subscriber Upgrades</title>.
> Some suggestions:
> 3a) Since all replication origins are now migrated, this:
>   "...greater than or equal to the number of subscriptions present in
> the old cluster."
> could become:
>   "...greater than or equal to the number of replication origins
> present in the old cluster."
>
> 3b) We should also mention:
>   The new cluster must contain no replication origins.
>

Done.



On Wed, Jun 17, 2026 at 9:42 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> 4) I was able to build successfully without all three new header
> inclusions below, so we may not need them.
> --- a/src/backend/utils/adt/pg_upgrade_support.c
> +++ b/src/backend/utils/adt/pg_upgrade_support.c
> @@ -11,6 +11,7 @@
> +#include "access/genam.h"
> ...
> +#include "utils/fmgroids.h"
> ...
> +#include "utils/snapmgr.h"
> ~~~
>

Removed.


> 5) Race condition in origin.c: replorigin_create()
> - heap_freetuple(tuple);
> + table_close(rel, ExclusiveLock);
> +
> + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
> +
>
> Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
> of a race between two concurrent CREATE SUBSCRIPTION commands:
>
> postgres=# create subscription sub2 connection 'dbname=postgres
> host=localhost port=7733' publication pub1;
> ERROR:  replication origin with ID 2 already exists
>
> I think this happens because the table lock is released, allowing
> another session to create a pg_origin with the next available roident,
> which replorigin_create_with_id() later attempts to use.
>
> Would it make sense to call replorigin_create_with_id() before
> table_close() so the lock is still held? I tested this and it works,
> though I'm not fully sure whether self-locking could be a concern
> here, since replorigin_create_with_id() again acquires a
> RowExclusiveLock on the same table.
> ~~~
>

I have fixed this as suggested by Shveta

~~~~~~~~~~



On Thu, Jun 18, 2026 at 7:17 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> > I think this happens because the table lock is released, allowing
> > another session to create a pg_origin with the next available roident,
> > which replorigin_create_with_id() later attempts to use.
> >
> > Would it make sense to call replorigin_create_with_id() before
> > table_close() so the lock is still held? I tested this and it works,
> > though I'm not fully sure whether self-locking could be a concern
> > here, since replorigin_create_with_id() again acquires a
> > RowExclusiveLock on the same table.
>
> We can handle this race-condition by moving the table-open to callers
> rather than internal call replorigin_create_with_id().  But we also
> need the following changes first:
>

Yes, did it that way.


> 1)
> replorigin_create_with_id has:
>
> + /*
> + * To avoid needing a TOAST table for pg_replication_origin, we limit
> + * replication origin names to 512 bytes.  This should be more than enough
> + * for all practical use.
> + */
> + if (strlen(roname) > MAX_RONAME_LEN)
> + ereport(ERROR,
> + errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> + errmsg("replication origin name is too long"),
> + errdetail("Replication origin names must be no longer than %d bytes.",
> +   MAX_RONAME_LEN));
>
> replorigin_create_with_id() is called by replorigin_create() and
> binary_upgrade_create_replication_origin().   replorigin_create()
> already has this check. We should move above check to
> binary_upgrade_create_replication_origin() and get rid of it from
> internal function replorigin_create_with_id().

Moved.

>
> 2)
>
> Also the checks:
> + if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(roident)))
> + ereport(ERROR,
> + errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin with ID %u already exists",
> +    (Oid) roident));
> +
> + if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
> + ereport(ERROR,
> + errcode(ERRCODE_DUPLICATE_OBJECT),
> + errmsg("replication origin \"%s\" already exists", roname));
>
> should be moved to caller binary_upgrade_create_replication_origin().
> IMO, these checks are not needed for replorigin_create(), it alread
> has this, which should suffice.
>
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
>                                  errmsg("could not find free
> replication origin ID")));
>
> ~~
>
> Once we correctly place these checks, we can move the 'table_open with
> lock' to caller (before name and id collision checks) instead of
> replorigin_create_with_id(), which will take care of race-conditions
> reported by Nisha.
>

I have moved the roident check out of replorigin_create_with_id(), but
kept the roname check there as that is not there in
replorigin_create()

On Fri, Jun 19, 2026 at 5:48 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> Hi Ajin,
>
> Thanks for providing the updated patch, I reviewed the patch and I
> have some comments:
>
> 1. patch is not applying on HEAD and needs a rebase.
>
> 2. New line after 'check_new_cluster_replication_origins' is not required:
> + check_new_cluster_replication_origins();
> +
>
> 3. pg_dumpall with option '--roles-only' also dumps replication origins
> eg:
> ./pg_dumpall -p 9000 --binary-upgrade --roles-only > op.sql
> output contains:
> -- For binary upgrade, must preserve replication origin roident and remote_lsn
> SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
>
> I think in this case we should not dump the replication origins as we
> explicitly gave the option of '--roles-only'
>

I have moved the dump_replication_origins under !roles_only and
!tablespaces_only checks, I think "globals only" is valid for
replication origins.


> 4. pg_dumpall with --globals-only option also dumps replication origins.
> eg:
> ./pg_dumpall -p 9000 --binary-upgrade --globals-only> op.sql and
> output contains:
> -- For binary upgrade, must preserve replication origin roident and remote_lsn
> SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
>
> Is this behaviour intentional? Other logical replication objects
> (e.g., publications and subscriptions) are not included in pg_dumpall
> --globals-only, so is it okay to dump replication origin?
>

This is expected as publications and subscriptions are database
specific and not globals, while replication origins are global.



On Mon, Jun 22, 2026 at 2:22 AM Rui Zhao <zhaorui126@gmail.com> wrote:
>>
>> Hi Ajin,
>>
>> Thanks for v8. A couple of comments:
>>
>> 1) The PG17 gating seems inconsistent between the dump and the check now.
>> dumpReplicationOrigins() is called unconditionally:
>>
>>     if (binary_upgrade && archDumpFormat == archNull)
>>         dumpReplicationOrigins(conn);
>>
>> but get_subscription_info(), which sets nrepl_origins, is still only
>> called for old clusters >= 17 in check_new_cluster():
>>
>>     if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
>>         ...
>>         get_subscription_info(&old_cluster);
>>
>> and check_new_cluster_replication_origins() returns early on it:
>>
>>     if (old_cluster.nrepl_origins == 0)
>>         return;
>>
>> For a < 17 old cluster get_subscription_info() is never called, so
>> nrepl_origins stays 0 and this returns early. So when such a cluster has a
>> (user-created) origin, the origin still gets dumped and recreated, while
>> the "new cluster must have no origins" and max_active_replication_origins
>> checks are skipped, and the upgrade fails during restore instead of at the
>> check step. nrepl_origins is just a count(*) on pg_replication_origin, so
>> moving its collection out of the >= 1700 block should be enough.
>>

Yes, good catch. I have separated out the replication origin speciifc
checks into a different function and moved it oustide the >= 1700
block

>> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Since
>> the patch migrates all origins, should we also create a user origin
>> (pg_replication_origin_create()) and verify its roident/remote_lsn after
>> the upgrade? That would also cover the case in (1).


Added.

All the above changes have been addressed in patch v9.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Rui Zhao <zhaorui126@gmail.com> — 2026-06-23T16:07:23Z

Hi Ajin,

Thanks for v9, and for addressing both my earlier comments. I also went
through the rest of v9 -- the other reviewers' comments and the
corresponding fixes (including the replorigin_create() locking rework) look
good to me; I didn't find any issues there.

A follow-up on the version gating, though: I think it now moved a bit too
far. get_replication_origin_info() is called unconditionally (outside the
>= 1700 block):

    /* Get replication origin information */
    get_replication_origin_info(&old_cluster);

and it runs "SELECT count(*) FROM pg_catalog.pg_replication_origin".
pg_dumpall's dumpReplicationOrigins() queries the same catalog with no
version guard either. But pg_replication_origin only exists from 9.5
(commit 5aa2350426), while pg_upgrade still accepts old clusters back to
9.2:

    if (GET_MAJOR_VERSION(old_cluster.major_version) < 902)
        pg_fatal("This utility can only upgrade from PostgreSQL version %s
                 and later.", "9.2");

So upgrading from 9.2/9.3/9.4 now fails: the query hits a non-existent
catalog and pg_fatal()s (and dumpReplicationOrigins would too).
It should be gated on the version where origins were introduced, e.g.

    if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
        get_replication_origin_info(&old_cluster);

with the matching server_version >= 90500 check on the pg_dumpall side.
That keeps the < 17 case I flagged working while not breaking 9.2-9.4
(which have no origins to migrate anyway).

One nit while here: there is an extra blank line after the
check_new_cluster_replication_origins() call in check_new_cluster().

Thanks,
Rui

Ajin Cherian <itsajin@gmail.com> 于2026年6月23日周二 19:50写道：
>
> On Wed, Jun 17, 2026 at 8:59 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Hi Ajin, Thanks for the patches.
> > I have started reviewing the patches. Please find a few initial
> > comments below (from v7 review, still applicable to v8):
> >
> > 1) Patch-002: pg_dump.c
> > + if (dopt->binary_upgrade && subinfo->subenabled &&
> > fout->remoteVersion >= 170000)
> >   {
> > - if (subinfo->suboriginremotelsn)
> > - {
> > - /*
> > - * Preserve the remote_lsn for the subscriber's replication
> >
> > I could not find any usage of subinfo[i].suboriginremotelsn anymore.
> > It seems to be dead code now. Can we remove it?
>
> Removed it.
>
>
> > ~~~
> >
> > 2) Patch-002 : binary_upgrade_create_replication_origin()
> > - PG_RETURN_VOID();
> > + node_oid = PG_GETARG_OID(0);
> > +
> > + if (node_oid == InvalidOid || node_oid > PG_UINT16_MAX)
> > + ereport(ERROR,
> > + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("replication origin ID %u is out of range", node_oid));
> > +
> >
> > The above range check allows PG_UINT16_MAX, but that value is not part
> > of the valid replication origin ID range.
> >
> > In origin.h:
> >   #define DoNotReplicateId PG_UINT16_MAX
> >
> > And we already ensure "ReplOriginId != DoNotReplicateId" everywhere
> > else. Would something like the below be more appropriate?
> >   if (node_oid == InvalidReplOriginId || node_oid >= DoNotReplicateId)
> >     ereport(ERROR, ...
> > ~~~
>
> Done.
>
>
> >
> > 3) I think we should add some additional details in
> > logical-replication.sgml, especially under <title>Prepare for
> > Subscriber Upgrades</title>.
> > Some suggestions:
> > 3a) Since all replication origins are now migrated, this:
> >   "...greater than or equal to the number of subscriptions present in
> > the old cluster."
> > could become:
> >   "...greater than or equal to the number of replication origins
> > present in the old cluster."
> >
> > 3b) We should also mention:
> >   The new cluster must contain no replication origins.
> >
>
> Done.
>
>
>
> On Wed, Jun 17, 2026 at 9:42 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > 4) I was able to build successfully without all three new header
> > inclusions below, so we may not need them.
> > --- a/src/backend/utils/adt/pg_upgrade_support.c
> > +++ b/src/backend/utils/adt/pg_upgrade_support.c
> > @@ -11,6 +11,7 @@
> > +#include "access/genam.h"
> > ...
> > +#include "utils/fmgroids.h"
> > ...
> > +#include "utils/snapmgr.h"
> > ~~~
> >
>
> Removed.
>
>
> > 5) Race condition in origin.c: replorigin_create()
> > - heap_freetuple(tuple);
> > + table_close(rel, ExclusiveLock);
> > +
> > + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
> > +
> >
> > Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
> > of a race between two concurrent CREATE SUBSCRIPTION commands:
> >
> > postgres=# create subscription sub2 connection 'dbname=postgres
> > host=localhost port=7733' publication pub1;
> > ERROR:  replication origin with ID 2 already exists
> >
> > I think this happens because the table lock is released, allowing
> > another session to create a pg_origin with the next available roident,
> > which replorigin_create_with_id() later attempts to use.
> >
> > Would it make sense to call replorigin_create_with_id() before
> > table_close() so the lock is still held? I tested this and it works,
> > though I'm not fully sure whether self-locking could be a concern
> > here, since replorigin_create_with_id() again acquires a
> > RowExclusiveLock on the same table.
> > ~~~
> >
>
> I have fixed this as suggested by Shveta
>
> ~~~~~~~~~~
>
>
>
> On Thu, Jun 18, 2026 at 7:17 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > > I think this happens because the table lock is released, allowing
> > > another session to create a pg_origin with the next available roident,
> > > which replorigin_create_with_id() later attempts to use.
> > >
> > > Would it make sense to call replorigin_create_with_id() before
> > > table_close() so the lock is still held? I tested this and it works,
> > > though I'm not fully sure whether self-locking could be a concern
> > > here, since replorigin_create_with_id() again acquires a
> > > RowExclusiveLock on the same table.
> >
> > We can handle this race-condition by moving the table-open to callers
> > rather than internal call replorigin_create_with_id().  But we also
> > need the following changes first:
> >
>
> Yes, did it that way.
>
>
> > 1)
> > replorigin_create_with_id has:
> >
> > + /*
> > + * To avoid needing a TOAST table for pg_replication_origin, we limit
> > + * replication origin names to 512 bytes.  This should be more than enough
> > + * for all practical use.
> > + */
> > + if (strlen(roname) > MAX_RONAME_LEN)
> > + ereport(ERROR,
> > + errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> > + errmsg("replication origin name is too long"),
> > + errdetail("Replication origin names must be no longer than %d bytes.",
> > +   MAX_RONAME_LEN));
> >
> > replorigin_create_with_id() is called by replorigin_create() and
> > binary_upgrade_create_replication_origin().   replorigin_create()
> > already has this check. We should move above check to
> > binary_upgrade_create_replication_origin() and get rid of it from
> > internal function replorigin_create_with_id().
>
> Moved.
>
> >
> > 2)
> >
> > Also the checks:
> > + if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(roident)))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists",
> > +    (Oid) roident));
> > +
> > + if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin \"%s\" already exists", roname));
> >
> > should be moved to caller binary_upgrade_create_replication_origin().
> > IMO, these checks are not needed for replorigin_create(), it alread
> > has this, which should suffice.
> >
> >                 ereport(ERROR,
> >                                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> >                                  errmsg("could not find free
> > replication origin ID")));
> >
> > ~~
> >
> > Once we correctly place these checks, we can move the 'table_open with
> > lock' to caller (before name and id collision checks) instead of
> > replorigin_create_with_id(), which will take care of race-conditions
> > reported by Nisha.
> >
>
> I have moved the roident check out of replorigin_create_with_id(), but
> kept the roname check there as that is not there in
> replorigin_create()
>
> On Fri, Jun 19, 2026 at 5:48 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > Hi Ajin,
> >
> > Thanks for providing the updated patch, I reviewed the patch and I
> > have some comments:
> >
> > 1. patch is not applying on HEAD and needs a rebase.
> >
> > 2. New line after 'check_new_cluster_replication_origins' is not required:
> > + check_new_cluster_replication_origins();
> > +
> >
> > 3. pg_dumpall with option '--roles-only' also dumps replication origins
> > eg:
> > ./pg_dumpall -p 9000 --binary-upgrade --roles-only > op.sql
> > output contains:
> > -- For binary upgrade, must preserve replication origin roident and remote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
> >
> > I think in this case we should not dump the replication origins as we
> > explicitly gave the option of '--roles-only'
> >
>
> I have moved the dump_replication_origins under !roles_only and
> !tablespaces_only checks, I think "globals only" is valid for
> replication origins.
>
>
> > 4. pg_dumpall with --globals-only option also dumps replication origins.
> > eg:
> > ./pg_dumpall -p 9000 --binary-upgrade --globals-only> op.sql and
> > output contains:
> > -- For binary upgrade, must preserve replication origin roident and remote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
> >
> > Is this behaviour intentional? Other logical replication objects
> > (e.g., publications and subscriptions) are not included in pg_dumpall
> > --globals-only, so is it okay to dump replication origin?
> >
>
> This is expected as publications and subscriptions are database
> specific and not globals, while replication origins are global.
>
>
>
> On Mon, Jun 22, 2026 at 2:22 AM Rui Zhao <zhaorui126@gmail.com> wrote:
> >>
> >> Hi Ajin,
> >>
> >> Thanks for v8. A couple of comments:
> >>
> >> 1) The PG17 gating seems inconsistent between the dump and the check now.
> >> dumpReplicationOrigins() is called unconditionally:
> >>
> >>     if (binary_upgrade && archDumpFormat == archNull)
> >>         dumpReplicationOrigins(conn);
> >>
> >> but get_subscription_info(), which sets nrepl_origins, is still only
> >> called for old clusters >= 17 in check_new_cluster():
> >>
> >>     if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> >>         ...
> >>         get_subscription_info(&old_cluster);
> >>
> >> and check_new_cluster_replication_origins() returns early on it:
> >>
> >>     if (old_cluster.nrepl_origins == 0)
> >>         return;
> >>
> >> For a < 17 old cluster get_subscription_info() is never called, so
> >> nrepl_origins stays 0 and this returns early. So when such a cluster has a
> >> (user-created) origin, the origin still gets dumped and recreated, while
> >> the "new cluster must have no origins" and max_active_replication_origins
> >> checks are skipped, and the upgrade fails during restore instead of at the
> >> check step. nrepl_origins is just a count(*) on pg_replication_origin, so
> >> moving its collection out of the >= 1700 block should be enough.
> >>
>
> Yes, good catch. I have separated out the replication origin speciifc
> checks into a different function and moved it oustide the >= 1700
> block
>
> >> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Since
> >> the patch migrates all origins, should we also create a user origin
> >> (pg_replication_origin_create()) and verify its roident/remote_lsn after
> >> the upgrade? That would also cover the case in (1).
>
>
> Added.
>
> All the above changes have been addressed in patch v9.
>
> regards,
> Ajin Cherian
> Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-24T05:09:00Z

On Tue, Jun 23, 2026 at 5:20 PM Ajin Cherian <itsajin@gmail.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

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-06-24T05:21:20Z

On Wed, Jun 24, 2026 at 10:39 AM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> 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.
>

Analyzed a bit more, with this change in patch002, we see an error change:

Without patch:
postgres=# SELECT pg_replication_origin_create('abcd');
 pg_replication_origin_create
------------------------------
                            1
postgres=# SELECT pg_replication_origin_create('abcd');
ERROR:  duplicate key value violates unique constraint
"pg_replication_origin_roname_index"
DETAIL:  Key (roname)=(abcd) already exists.

----

With patch:
postgres=#  SELECT pg_replication_origin_create('abcd');
 pg_replication_origin_create
------------------------------
                            1

postgres=# SELECT pg_replication_origin_create('abcd');
ERROR:  replication origin "abcd" already exists


So without the new check also, we can prevent duplicate name insertion
(even if that happens during upgrade, which I can not see  how). The
new error and the check is not wrong, but slightly redundant. Or let
me know if I am missing something.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-06-24T13:27:44Z

On Tue, 23 Jun 2026 at 17:20, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Jun 17, 2026 at 8:59 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Hi Ajin, Thanks for the patches.
> > I have started reviewing the patches. Please find a few initial
> > comments below (from v7 review, still applicable to v8):
> >
> > 1) Patch-002: pg_dump.c
> > + if (dopt->binary_upgrade && subinfo->subenabled &&
> > fout->remoteVersion >= 170000)
> >   {
> > - if (subinfo->suboriginremotelsn)
> > - {
> > - /*
> > - * Preserve the remote_lsn for the subscriber's replication
> >
> > I could not find any usage of subinfo[i].suboriginremotelsn anymore.
> > It seems to be dead code now. Can we remove it?
>
> Removed it.
>
>
> > ~~~
> >
> > 2) Patch-002 : binary_upgrade_create_replication_origin()
> > - PG_RETURN_VOID();
> > + node_oid = PG_GETARG_OID(0);
> > +
> > + if (node_oid == InvalidOid || node_oid > PG_UINT16_MAX)
> > + ereport(ERROR,
> > + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("replication origin ID %u is out of range", node_oid));
> > +
> >
> > The above range check allows PG_UINT16_MAX, but that value is not part
> > of the valid replication origin ID range.
> >
> > In origin.h:
> >   #define DoNotReplicateId PG_UINT16_MAX
> >
> > And we already ensure "ReplOriginId != DoNotReplicateId" everywhere
> > else. Would something like the below be more appropriate?
> >   if (node_oid == InvalidReplOriginId || node_oid >= DoNotReplicateId)
> >     ereport(ERROR, ...
> > ~~~
>
> Done.
>
>
> >
> > 3) I think we should add some additional details in
> > logical-replication.sgml, especially under <title>Prepare for
> > Subscriber Upgrades</title>.
> > Some suggestions:
> > 3a) Since all replication origins are now migrated, this:
> >   "...greater than or equal to the number of subscriptions present in
> > the old cluster."
> > could become:
> >   "...greater than or equal to the number of replication origins
> > present in the old cluster."
> >
> > 3b) We should also mention:
> >   The new cluster must contain no replication origins.
> >
>
> Done.
>
>
>
> On Wed, Jun 17, 2026 at 9:42 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > 4) I was able to build successfully without all three new header
> > inclusions below, so we may not need them.
> > --- a/src/backend/utils/adt/pg_upgrade_support.c
> > +++ b/src/backend/utils/adt/pg_upgrade_support.c
> > @@ -11,6 +11,7 @@
> > +#include "access/genam.h"
> > ...
> > +#include "utils/fmgroids.h"
> > ...
> > +#include "utils/snapmgr.h"
> > ~~~
> >
>
> Removed.
>
>
> > 5) Race condition in origin.c: replorigin_create()
> > - heap_freetuple(tuple);
> > + table_close(rel, ExclusiveLock);
> > +
> > + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
> > +
> >
> > Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
> > of a race between two concurrent CREATE SUBSCRIPTION commands:
> >
> > postgres=# create subscription sub2 connection 'dbname=postgres
> > host=localhost port=7733' publication pub1;
> > ERROR:  replication origin with ID 2 already exists
> >
> > I think this happens because the table lock is released, allowing
> > another session to create a pg_origin with the next available roident,
> > which replorigin_create_with_id() later attempts to use.
> >
> > Would it make sense to call replorigin_create_with_id() before
> > table_close() so the lock is still held? I tested this and it works,
> > though I'm not fully sure whether self-locking could be a concern
> > here, since replorigin_create_with_id() again acquires a
> > RowExclusiveLock on the same table.
> > ~~~
> >
>
> I have fixed this as suggested by Shveta
>
> ~~~~~~~~~~
>
>
>
> On Thu, Jun 18, 2026 at 7:17 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > > I think this happens because the table lock is released, allowing
> > > another session to create a pg_origin with the next available roident,
> > > which replorigin_create_with_id() later attempts to use.
> > >
> > > Would it make sense to call replorigin_create_with_id() before
> > > table_close() so the lock is still held? I tested this and it works,
> > > though I'm not fully sure whether self-locking could be a concern
> > > here, since replorigin_create_with_id() again acquires a
> > > RowExclusiveLock on the same table.
> >
> > We can handle this race-condition by moving the table-open to callers
> > rather than internal call replorigin_create_with_id().  But we also
> > need the following changes first:
> >
>
> Yes, did it that way.
>
>
> > 1)
> > replorigin_create_with_id has:
> >
> > + /*
> > + * To avoid needing a TOAST table for pg_replication_origin, we limit
> > + * replication origin names to 512 bytes.  This should be more than enough
> > + * for all practical use.
> > + */
> > + if (strlen(roname) > MAX_RONAME_LEN)
> > + ereport(ERROR,
> > + errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> > + errmsg("replication origin name is too long"),
> > + errdetail("Replication origin names must be no longer than %d bytes.",
> > +   MAX_RONAME_LEN));
> >
> > replorigin_create_with_id() is called by replorigin_create() and
> > binary_upgrade_create_replication_origin().   replorigin_create()
> > already has this check. We should move above check to
> > binary_upgrade_create_replication_origin() and get rid of it from
> > internal function replorigin_create_with_id().
>
> Moved.
>
> >
> > 2)
> >
> > Also the checks:
> > + if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(roident)))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists",
> > +    (Oid) roident));
> > +
> > + if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin \"%s\" already exists", roname));
> >
> > should be moved to caller binary_upgrade_create_replication_origin().
> > IMO, these checks are not needed for replorigin_create(), it alread
> > has this, which should suffice.
> >
> >                 ereport(ERROR,
> >                                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> >                                  errmsg("could not find free
> > replication origin ID")));
> >
> > ~~
> >
> > Once we correctly place these checks, we can move the 'table_open with
> > lock' to caller (before name and id collision checks) instead of
> > replorigin_create_with_id(), which will take care of race-conditions
> > reported by Nisha.
> >
>
> I have moved the roident check out of replorigin_create_with_id(), but
> kept the roname check there as that is not there in
> replorigin_create()
>
> On Fri, Jun 19, 2026 at 5:48 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > Hi Ajin,
> >
> > Thanks for providing the updated patch, I reviewed the patch and I
> > have some comments:
> >
> > 1. patch is not applying on HEAD and needs a rebase.
> >
> > 2. New line after 'check_new_cluster_replication_origins' is not required:
> > + check_new_cluster_replication_origins();
> > +
> >
> > 3. pg_dumpall with option '--roles-only' also dumps replication origins
> > eg:
> > ./pg_dumpall -p 9000 --binary-upgrade --roles-only > op.sql
> > output contains:
> > -- For binary upgrade, must preserve replication origin roident and remote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
> >
> > I think in this case we should not dump the replication origins as we
> > explicitly gave the option of '--roles-only'
> >
>
> I have moved the dump_replication_origins under !roles_only and
> !tablespaces_only checks, I think "globals only" is valid for
> replication origins.
>
>
> > 4. pg_dumpall with --globals-only option also dumps replication origins.
> > eg:
> > ./pg_dumpall -p 9000 --binary-upgrade --globals-only> op.sql and
> > output contains:
> > -- For binary upgrade, must preserve replication origin roident and remote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> > 'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);
> >
> > Is this behaviour intentional? Other logical replication objects
> > (e.g., publications and subscriptions) are not included in pg_dumpall
> > --globals-only, so is it okay to dump replication origin?
> >
>
> This is expected as publications and subscriptions are database
> specific and not globals, while replication origins are global.
>
>
>
> On Mon, Jun 22, 2026 at 2:22 AM Rui Zhao <zhaorui126@gmail.com> wrote:
> >>
> >> Hi Ajin,
> >>
> >> Thanks for v8. A couple of comments:
> >>
> >> 1) The PG17 gating seems inconsistent between the dump and the check now.
> >> dumpReplicationOrigins() is called unconditionally:
> >>
> >>     if (binary_upgrade && archDumpFormat == archNull)
> >>         dumpReplicationOrigins(conn);
> >>
> >> but get_subscription_info(), which sets nrepl_origins, is still only
> >> called for old clusters >= 17 in check_new_cluster():
> >>
> >>     if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> >>         ...
> >>         get_subscription_info(&old_cluster);
> >>
> >> and check_new_cluster_replication_origins() returns early on it:
> >>
> >>     if (old_cluster.nrepl_origins == 0)
> >>         return;
> >>
> >> For a < 17 old cluster get_subscription_info() is never called, so
> >> nrepl_origins stays 0 and this returns early. So when such a cluster has a
> >> (user-created) origin, the origin still gets dumped and recreated, while
> >> the "new cluster must have no origins" and max_active_replication_origins
> >> checks are skipped, and the upgrade fails during restore instead of at the
> >> check step. nrepl_origins is just a count(*) on pg_replication_origin, so
> >> moving its collection out of the >= 1700 block should be enough.
> >>
>
> Yes, good catch. I have separated out the replication origin speciifc
> checks into a different function and moved it oustide the >= 1700
> block
>
> >> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Since
> >> the patch migrates all origins, should we also create a user origin
> >> (pg_replication_origin_create()) and verify its roident/remote_lsn after
> >> the upgrade? That would also cover the case in (1).
>
>
> Added.
>
> All the above changes have been addressed in patch v9.
>
Thanks for the updated patch. I have reviewed the patch and here are
some my comments:

1. Since we are quering only one column, do we need to use 'i_norigins'?

+void
+get_replication_origin_info(ClusterInfo *cluster)
+{
+ PGconn    *conn;
+ PGresult   *res;
+ int i_norigins;
+
+ conn = connectToServer(cluster, "template1");
+ res = executeQueryOrDie(conn, "SELECT count(*) AS norigins "
+    "FROM pg_catalog.pg_replication_origin");
+ i_norigins = PQfnumber(res, "norigins");
+
+ cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_norigins));
+ PQclear(res);
+
+ PQfinish(conn);
+}
I think we can directly use:
cluster->nrepl_origins = atoi(PQgetvalue(res, 0, 0));

Thoughts?

2. In the above code should we also add a check like:
if (PQntuples(res) != 1)
pg_fatal("could not count the number of replication origins");

3. The brackets are not required:
+ if (!tablespaces_only && !roles_only && binary_upgrade)
+ {
+ /* Dump replication origins */
+ dumpReplicationOrigins(conn);
  }
We can move the comment '/* Dump replication origins */' above if condition.
It will make it consistent with the existing code.

4. I ran the tests locally and found that the test_decoding test suit
is failing:
not ok 10    - replorigin                                249 ms

diff:
 -- ensure duplicate creations fail
 SELECT pg_replication_origin_create('regress_test_decoding: regression_slot');
-ERROR:  duplicate key value violates unique constraint
"pg_replication_origin_roname_index"
-DETAIL:  Key (roname)=(regress_test_decoding: regression_slot) already exists.
+ERROR:  replication origin "regress_test_decoding: regression_slot"
already exists
 -- ensure inactive origin cannot be set as session one if pid is specified
 SELECT pg_replication_origin_session_setup('regress_test_decoding:
regression_slot', -1);
 ERROR:  cannot use PID -1 for inactive replication origin with ID 1

I think this is expected because this patch adds a new function
'replorigin_create_with_id'
which adds a check for the already existing replication origin.
In HEAD we don't have such a check. So I think we should update the
expected output file.

Thanks,
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Zsolt Parragi <zsolt.parragi@percona.com> — 2026-06-24T16:44:52Z

Hello

+# Verify that the subscription related replication origins are
preserved after upgrade.
+my $post_roident_rows = $new_sub->safe_psql('postgres',
+    "SELECT s.subname, o.roident
+     FROM pg_subscription s
+     JOIN pg_replication_origin o ON o.roname = 'pg_' || s.oid::text
+     ORDER BY s.subname"
+);
+for my $row (split /\n/, $post_roident_rows)
+{
+    my ($subname, $roident) = split /\|/, $row;
+    is($roident, $pre_upgrade_roident{$subname},
+        "roident preserved for subscription '$subname' after upgrade");
+}
+

Won't this check miss (and silently pass) if an entry is missing
post-upgrade? It verifies that all entries that exists match the pre
upgrade entries, but it doesn't verify that the list is the same.

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Ajin Cherian <itsajin@gmail.com> — 2026-07-01T02:49:49Z

On Wed, Jun 24, 2026 at 2:07 AM Rui Zhao <zhaorui126@gmail.com> wrote:
>
> Hi Ajin,
>
> Thanks for v9, and for addressing both my earlier comments. I also went
> through the rest of v9 -- the other reviewers' comments and the
> corresponding fixes (including the replorigin_create() locking rework) look
> good to me; I didn't find any issues there.
>
> A follow-up on the version gating, though: I think it now moved a bit too
> far. get_replication_origin_info() is called unconditionally (outside the
> >= 1700 block):
>
>     /* Get replication origin information */
>     get_replication_origin_info(&old_cluster);
>
> and it runs "SELECT count(*) FROM pg_catalog.pg_replication_origin".
> pg_dumpall's dumpReplicationOrigins() queries the same catalog with no
> version guard either. But pg_replication_origin only exists from 9.5
> (commit 5aa2350426), while pg_upgrade still accepts old clusters back to
> 9.2:
>
>     if (GET_MAJOR_VERSION(old_cluster.major_version) < 902)
>         pg_fatal("This utility can only upgrade from PostgreSQL version %s
>                  and later.", "9.2");
>
> So upgrading from 9.2/9.3/9.4 now fails: the query hits a non-existent
> catalog and pg_fatal()s (and dumpReplicationOrigins would too).
> It should be gated on the version where origins were introduced, e.g.
>
>     if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
>         get_replication_origin_info(&old_cluster);
>
> with the matching server_version >= 90500 check on the pg_dumpall side.
> That keeps the < 17 case I flagged working while not breaking 9.2-9.4
> (which have no origins to migrate anyway).
>

Thanks, I have added both the flags.

> One nit while here: there is an extra blank line after the
> check_new_cluster_replication_origins() call in check_new_cluster().
>

Removed.


On Wed, Jun 24, 2026 at 3:09 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Jun 23, 2026 at 5:20 PM Ajin Cherian <itsajin@gmail.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
>

Changed.

> 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.
>

I've removed 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(..)
> }
>

Done.

> 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.
>

I've removed found and instead checked if the roident has reached the max limit

> 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.
>

Removed

> 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.

I've removed nsubs




On Wed, Jun 24, 2026 at 3:21 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jun 24, 2026 at 10:39 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > 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.
> >
>
> Analyzed a bit more, with this change in patch002, we see an error change:
>
> Without patch:
> postgres=# SELECT pg_replication_origin_create('abcd');
>  pg_replication_origin_create
> ------------------------------
>                             1
> postgres=# SELECT pg_replication_origin_create('abcd');
> ERROR:  duplicate key value violates unique constraint
> "pg_replication_origin_roname_index"
> DETAIL:  Key (roname)=(abcd) already exists.
>
> ----
>
> With patch:
> postgres=#  SELECT pg_replication_origin_create('abcd');
>  pg_replication_origin_create
> ------------------------------
>                             1
>
> postgres=# SELECT pg_replication_origin_create('abcd');
> ERROR:  replication origin "abcd" already exists
>
>
> So without the new check also, we can prevent duplicate name insertion
> (even if that happens during upgrade, which I can not see  how). The
> new error and the check is not wrong, but slightly redundant. Or let
> me know if I am missing something.
>

I've removed that check.




On Wed, Jun 24, 2026 at 11:27 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Thanks for the updated patch. I have reviewed the patch and here are
> some my comments:
>
> 1. Since we are quering only one column, do we need to use 'i_norigins'?
>
> +void
> +get_replication_origin_info(ClusterInfo *cluster)
> +{
> + PGconn    *conn;
> + PGresult   *res;
> + int i_norigins;
> +
> + conn = connectToServer(cluster, "template1");
> + res = executeQueryOrDie(conn, "SELECT count(*) AS norigins "
> +    "FROM pg_catalog.pg_replication_origin");
> + i_norigins = PQfnumber(res, "norigins");
> +
> + cluster->nrepl_origins = atoi(PQgetvalue(res, 0, i_norigins));
> + PQclear(res);
> +
> + PQfinish(conn);
> +}
> I think we can directly use:
> cluster->nrepl_origins = atoi(PQgetvalue(res, 0, 0));
>
> Thoughts?

Removed it.

>
> 2. In the above code should we also add a check like:
> if (PQntuples(res) != 1)
> pg_fatal("could not count the number of replication origins");
>

Added.

> 3. The brackets are not required:
> + if (!tablespaces_only && !roles_only && binary_upgrade)
> + {
> + /* Dump replication origins */
> + dumpReplicationOrigins(conn);
>   }
> We can move the comment '/* Dump replication origins */' above if condition.
> It will make it consistent with the existing code.
>

I've moved the comment up but keeping the brackets as it has more than
one line, that is the recommendation from Tom Lane.

> 4. I ran the tests locally and found that the test_decoding test suit
> is failing:
> not ok 10    - replorigin                                249 ms
>
> diff:
>  -- ensure duplicate creations fail
>  SELECT pg_replication_origin_create('regress_test_decoding: regression_slot');
> -ERROR:  duplicate key value violates unique constraint
> "pg_replication_origin_roname_index"
> -DETAIL:  Key (roname)=(regress_test_decoding: regression_slot) already exists.
> +ERROR:  replication origin "regress_test_decoding: regression_slot"
> already exists
>  -- ensure inactive origin cannot be set as session one if pid is specified
>  SELECT pg_replication_origin_session_setup('regress_test_decoding:
> regression_slot', -1);
>  ERROR:  cannot use PID -1 for inactive replication origin with ID 1
>
> I think this is expected because this patch adds a new function
> 'replorigin_create_with_id'
> which adds a check for the already existing replication origin.
> In HEAD we don't have such a check. So I think we should update the
> expected output file.
>

Based on feedback from Shveta, I've removed that new error as changing
existing behaviour isn't required.



On Thu, Jun 25, 2026 at 2:45 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:
>
> Hello
>
> +# Verify that the subscription related replication origins are
> preserved after upgrade.
> +my $post_roident_rows = $new_sub->safe_psql('postgres',
> +    "SELECT s.subname, o.roident
> +     FROM pg_subscription s
> +     JOIN pg_replication_origin o ON o.roname = 'pg_' || s.oid::text
> +     ORDER BY s.subname"
> +);
> +for my $row (split /\n/, $post_roident_rows)
> +{
> +    my ($subname, $roident) = split /\|/, $row;
> +    is($roident, $pre_upgrade_roident{$subname},
> +        "roident preserved for subscription '$subname' after upgrade");
> +}
> +
>
> Won't this check miss (and silently pass) if an entry is missing
> post-upgrade? It verifies that all entries that exists match the pre
> upgrade entries, but it doesn't verify that the list is the same.

Modified this to iterate over pre upgrade and post upgrade
subscriptions making sure both are present.

All the above changes are updated in patch v10.

regards,
Ajin Cherian
Fujitsu Australia

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Rui Zhao <zhaorui126@gmail.com> — 2026-07-05T16:09:55Z

Hi Ajin,

Thanks for v10. On the version gating I asked about earlier -- I think it's
now moot, and the gates can come back out.

Since v10, 14d8418083 ("Remove pg_upgrade support for upgrading from pre-v10
servers") raised pg_upgrade's minimum source version to v10.
pg_replication_origin has existed since 9.5, so every cluster pg_upgrade now
accepts already has it, and the original unconditional
get_replication_origin_info() is safe again. Both gates that went into v10 are
now always true:

  - check.c:      if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
  - pg_dumpall.c: if (server_version >= 90500)   -- only reached under
                  binary_upgrade, i.e. against the v10+ old cluster

14d8418083 was itself removing exactly this kind of pre-v10 version check
across pg_upgrade, so dropping these fits the same cleanup.

v10 doesn't apply on current master anyway (0002 fails on pg_proc.dat), so
could you post a rebase? That'd let it be built and tested against HEAD, and
it's a natural point to drop the two gates and go back to the unconditional
calls.

Minor, in that same hunk: the dumpReplicationOrigins(conn) call isn't indented
under its if.

Otherwise v10 reads well; the other reviewers' comments all look addressed.

Zsolt, a small request -- could you use reply-all on the thread? A couple of
times the CC list got narrowed, and when the reply chain continued from there
I dropped off it and missed the update. Keeping everyone CC'd helps the rest
of us stay in the loop.

Thanks,
Rui

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-07-06T03:31:26Z

On Sun, Jul 5, 2026 at 9:40 PM Rui Zhao <zhaorui126@gmail.com> wrote:
>
> Hi Ajin,
>
> Thanks for v10. On the version gating I asked about earlier -- I think it's
> now moot, and the gates can come back out.
>
> Since v10, 14d8418083 ("Remove pg_upgrade support for upgrading from pre-v10
> servers") raised pg_upgrade's minimum source version to v10.
> pg_replication_origin has existed since 9.5, so every cluster pg_upgrade now
> accepts already has it, and the original unconditional
> get_replication_origin_info() is safe again. Both gates that went into v10 are
> now always true:
>
>   - check.c:      if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
>   - pg_dumpall.c: if (server_version >= 90500)   -- only reached under
>                   binary_upgrade, i.e. against the v10+ old cluster
>
> 14d8418083 was itself removing exactly this kind of pre-v10 version check
> across pg_upgrade, so dropping these fits the same cleanup.
>

Thanks for pointing to the commit.

> v10 doesn't apply on current master anyway (0002 fails on pg_proc.dat), so
> could you post a rebase? That'd let it be built and tested against HEAD, and
> it's a natural point to drop the two gates and go back to the unconditional
> calls.

I can apply v10 on the latest HEAD (9d1188f298), can you please try it again?

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-07-06T05:12:24Z

On Wed, Jul 1, 2026 at 8:20 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> All the above changes are updated in patch v10.
>

Thanks for the patch. I have not reviewed completely yet, but please
find comments so far:


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

Shall we slightly tweak it to:
/*
 * In binary-upgrade mode, skip origin creation here. It will be
 * created separately so that the origin ID (roident) from the
 * old cluster can be preserved for this subscription.
 */

dumpReplicationOrigins:
2)
+ appendPQExpBufferStr(buf,
+ "SELECT o.*, os.remote_lsn "

It will be better to be more specific here to make it more future proof:
SELECT o.roident, o.roname, os.remote_lsn

3)
+ if (PQntuples(res) > 0)
+ fprintf(OPF, "--\n-- Replication Origins \n--\n\n");
+
+ for (int i = 0; i < PQntuples(res); i++)

Instead of fetching it in loop, we can fetch it once and use it:
ntups = PQntuples(res)

4)
+ appendPQExpBufferStr(buf, "\n-- For binary upgrade, must preserve
replication origin roident and remote_lsn\n");

Seeing the pattern of other such comments in file, we can change
'origin' to be more specific 'pg_replication_origin'

5)
get_replication_origin_info:
+ res = executeQueryOrDie(conn, "SELECT count(*) AS norigins "
+    "FROM pg_catalog.pg_replication_origin");

I don't see alias 'norigins' being sued anywhere. We can get rid of it
if not needed.

6)
get_subscription_info: I think a bug is introduced with the new change.

Earlier query was below which will always return a row even if there
is no subscription entry:

postgres=# SELECT count(*) AS nsub,
       'f' AS retain_dead_tuples
FROM pg_catalog.pg_subscription;
 nsub | retain_dead_tuples
------+--------------------
    0 | f

Now, this will not even return a result if there are no subscription entries:

postgres=# SELECT 'f' AS retain_dead_tuples
FROM pg_catalog.pg_subscription;
 retain_dead_tuples
--------------------
(0 rows)

So this line of code will access a non existing result:

  i_retain_dead_tuples = PQfnumber(res, "retain_dead_tuples");


Should we simply do:
res = executeQueryOrDie(conn,
"SELECT false AS retain_dead_tuples");
(false/'f' anything is fine)

instead of:
res = executeQueryOrDie(conn,
"SELECT 'f' AS retain_dead_tuples "
"FROM pg_catalog.pg_subscription");

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Shlok Kyal <shlok.kyal.oss@gmail.com> — 2026-07-06T06:40:34Z

> All the above changes are updated in patch v10.
>
I noticed that we can create user defined replication origin even if the
total count of replication origins exceeds the max_active_replication_origins
Example:
postgres=# show max_active_replication_origins;
 max_active_replication_origins
--------------------------------
 1

postgres=# select * from pg_replication_origin;
 roident |  roname
---------+----------
       1 | pg_16387

postgres=# SELECT pg_replication_origin_create('origin1');
 pg_replication_origin_create
------------------------------
                            2

postgres=# select * from pg_replication_origin;
 roident |  roname
---------+----------
       1 | pg_16387
       2 | origin1

postgres=# select * from pg_replication_origin_status;
 local_id | external_id | remote_lsn | local_lsn
----------+-------------+------------+------------
        1 | pg_16387    | 0/00000000 | 0/00000000

The documentation for max_active_replication_origins says:
```
Specifies how many replication origins (see
<xref linkend="replication-origins"/>) can be tracked simultaneously,
effectively limiting how many logical replication subscriptions can
be created on the server. Setting it to a lower value than the current
number of tracked replication origins (reflected in
<link linkend="view-pg-replication-origin-status">pg_replication_origin_status</link>)
will prevent the server from starting. It defaults to 10. This parameter
can only be set at server start.
```
Since the user-created replication origin above is not
actively tracked in pg_replication_origin_status, no error is reported.

But with this patch, during pg_upgrade we are checking the
'max_active_replication_origins' against 'old_cluster.nrepl_origins'
+ if (old_cluster.nrepl_origins > max_active_replication_origins)
  pg_fatal("\"max_active_replication_origins\" (%d) must be greater
than or equal to the number of "
- "subscriptions (%d) in the old cluster",
- max_active_replication_origins, old_cluster.nsubs);
+ "replication origins (%d) in the old cluster",
+ max_active_replication_origins, old_cluster.nrepl_origins);

Since old_cluster.nrepl_origins counts all replication origins, including
user-created ones that may not be tracked, should this check instead compare
max_active_replication_origins with 'the number of rows in
pg_replication_origin_status in old cluster' (i.e., the number of
tracked replication origins)?

Thanks
Shlok Kyal

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Rui Zhao <zhaorui126@gmail.com> — 2026-07-06T13:10:18Z

Hi Ajin,

Shveta's right, sorry for the noise -- git am --3way applies v10 cleanly on
HEAD (9d1188f298) and it builds. "git apply --check" was tripping on the
shifted pg_proc.dat hunk on my end.

Confirmed Shveta's (6), and it's a crash, not just a bad read:

  cluster->sub_retain_dead_tuples = (strcmp(PQgetvalue(res, 0,
i_retain_dead_tuples), "t") == 0);

With no subscriptions the < 1900 query returns zero rows, PQgetvalue(res, 0,
...) returns NULL for a nonexistent row, and the strcmp segfaults (reproduced
with a small libpq program). Upgrading a v17/v18 cluster that has no
subscriptions hits it. Shveta's "SELECT false" fix is right.

A few more comments from re-reading v10:

1) The doc hunk updates this sentence:

    -     Commit timestamps and origin data are not preserved during
the upgrade.
    +     Commit timestamps are not preserved during the upgrade.

but the twin comment in pg_upgrade.c (~line 222) is left unchanged and needs
the same edit:

     * upgrade. Additionally, commit timestamps and origin data are not
     * preserved during the upgrade. So, even after creating the slot, the

2) dumpReplicationOrigins:

    + appendPQExpBufferStr(buf,
    + "SELECT o.*, os.remote_lsn "
    + "FROM pg_catalog.pg_replication_origin o "
    + "LEFT OUTER JOIN pg_catalog.pg_replication_origin_status os ON
o.roident = os.local_id ");

No ORDER BY, so the output order is whatever the heap returns; every other
query in pg_dumpall.c has one. ORDER BY o.roident would keep the dump stable.

3) Trivia: trailing space in the section header --

    + fprintf(OPF, "--\n-- Replication Origins \n--\n\n");

and a stray blank line at the end of replorigin_create_with_id():

    +    false /* WAL log */);
    + }
    +
    +}

Also, per my earlier mail, the >= 905 / >= 90500 gates can come out now that
14d8418083 raised the pg_upgrade floor to v10.

Thanks,
Rui

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-07-08T04:01:59Z

On Mon, Jul 6, 2026 at 12:10 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> > All the above changes are updated in patch v10.
> >
> I noticed that we can create user defined replication origin even if the
> total count of replication origins exceeds the max_active_replication_origins
> Example:
> postgres=# show max_active_replication_origins;
>  max_active_replication_origins
> --------------------------------
>  1
>
> postgres=# select * from pg_replication_origin;
>  roident |  roname
> ---------+----------
>        1 | pg_16387
>
> postgres=# SELECT pg_replication_origin_create('origin1');
>  pg_replication_origin_create
> ------------------------------
>                             2
>
> postgres=# select * from pg_replication_origin;
>  roident |  roname
> ---------+----------
>        1 | pg_16387
>        2 | origin1
>
> postgres=# select * from pg_replication_origin_status;
>  local_id | external_id | remote_lsn | local_lsn
> ----------+-------------+------------+------------
>         1 | pg_16387    | 0/00000000 | 0/00000000
>
> The documentation for max_active_replication_origins says:
> ```
> Specifies how many replication origins (see
> <xref linkend="replication-origins"/>) can be tracked simultaneously,
> effectively limiting how many logical replication subscriptions can
> be created on the server. Setting it to a lower value than the current
> number of tracked replication origins (reflected in
> <link linkend="view-pg-replication-origin-status">pg_replication_origin_status</link>)
> will prevent the server from starting. It defaults to 10. This parameter
> can only be set at server start.
> ```
> Since the user-created replication origin above is not
> actively tracked in pg_replication_origin_status, no error is reported.
>
> But with this patch, during pg_upgrade we are checking the
> 'max_active_replication_origins' against 'old_cluster.nrepl_origins'
> + if (old_cluster.nrepl_origins > max_active_replication_origins)
>   pg_fatal("\"max_active_replication_origins\" (%d) must be greater
> than or equal to the number of "
> - "subscriptions (%d) in the old cluster",
> - max_active_replication_origins, old_cluster.nsubs);
> + "replication origins (%d) in the old cluster",
> + max_active_replication_origins, old_cluster.nrepl_origins);
>
> Since old_cluster.nrepl_origins counts all replication origins, including
> user-created ones that may not be tracked, should this check instead compare
> max_active_replication_origins with 'the number of rows in
> pg_replication_origin_status in old cluster' (i.e., the number of
> tracked replication origins)?
>

Thanks Shlok for exploring this scenario.

I had a look at it. Please find my analysis.

I was under the assumption that subscription creation (which
internally creates a replication origin) would fail if
max_active_replication_origins was exceeded. However, that is not the
case. See the following experiment:

postgres=# show max_active_replication_origins;
 max_active_replication_origins
--------------------------------
 2

Al these commands succeed:

postgres=# create subscription sub1 ...
postgres=# create subscription sub2 ..
postgres=#  SELECT pg_replication_origin_create('user_origin1');
postgres=#  SELECT pg_replication_origin_create('user_origin2');
postgres=#  SELECT pg_replication_origin_create('user_origin3');
postgres=#  SELECT pg_replication_origin_create('user_origin4');
postgres=# create subscription sub3 ..
postgres=# create subscription sub4...

postgres=# select * from pg_replication_origin_status;
 local_id | external_id | remote_lsn | local_lsn
----------+-------------+------------+------------
        1 | pg_16384    | 0/00000000 | 0/00000000
        2 | pg_16385    | 0/00000000 | 0/00000000
(2 rows)

postgres=# select * from pg_replication_origin;
 roident |    roname
---------+--------------
       1 | pg_16384
       2 | pg_16385
       3 | user_origin1
       4 | user_origin2
       5 | user_origin3
       6 | user_origin4
       7 | pg_16387
       8 | pg_16388
(8 rows)

As shown above, sub3 and sub4 are created successfully, but their
replication origins are not tracked because there are no free active
replication origin slots. The apply workers for these subscriptions
also fail to start:

LOG:  logical replication apply worker for subscription "sub3" has started
ERROR:  could not find free replication state slot for replication
origin with ID 7
HINT:  Increase "max_active_replication_origins" and try again.
LOG:  background worker "logical replication apply worker" (PID
277193) exited with exit code 1

This means that if we implement the check suggested by Shlok:

Although pg_upgrade would succeed, it could leave some subscriptions
on the new cluster in non-functional state. I agree that this
situation (non-functional subscriptions) is also possible on the old
cluster. However, this would also deviate from the previous
implementation, where check_new_cluster_subscription_configuration()
compared old_cluster.nsubs against 'max_active_replication_origins' on
the new cluster, rather than comparing the number of entries in
pg_replication_origin_status.

For this reason, my inclination is to revert to the previous
implementation, where we compare old_cluster.nsubs with
max_active_replication_origins. But I'd like to hear what others think
before we make this change.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-07-08T04:07:51Z

On Mon, Jul 6, 2026 at 6:40 PM Rui Zhao <zhaorui126@gmail.com> wrote:
>
> Hi Ajin,
>
> Shveta's right, sorry for the noise -- git am --3way applies v10 cleanly on
> HEAD (9d1188f298) and it builds. "git apply --check" was tripping on the
> shifted pg_proc.dat hunk on my end.
>
> Confirmed Shveta's (6), and it's a crash, not just a bad read:

Thanks for verifying it.

>   cluster->sub_retain_dead_tuples = (strcmp(PQgetvalue(res, 0,
> i_retain_dead_tuples), "t") == 0);
>
> With no subscriptions the < 1900 query returns zero rows, PQgetvalue(res, 0,
> ...) returns NULL for a nonexistent row, and the strcmp segfaults (reproduced
> with a small libpq program). Upgrading a v17/v18 cluster that has no
> subscriptions hits it. Shveta's "SELECT false" fix is right.
>
> A few more comments from re-reading v10:
>
> 1) The doc hunk updates this sentence:
>
>     -     Commit timestamps and origin data are not preserved during
> the upgrade.
>     +     Commit timestamps are not preserved during the upgrade.
>
> but the twin comment in pg_upgrade.c (~line 222) is left unchanged and needs
> the same edit:
>
>      * upgrade. Additionally, commit timestamps and origin data are not
>      * preserved during the upgrade. So, even after creating the slot, the
>
> 2) dumpReplicationOrigins:
>
>     + appendPQExpBufferStr(buf,
>     + "SELECT o.*, os.remote_lsn "
>     + "FROM pg_catalog.pg_replication_origin o "
>     + "LEFT OUTER JOIN pg_catalog.pg_replication_origin_status os ON
> o.roident = os.local_id ");
>
> No ORDER BY, so the output order is whatever the heap returns; every other
> query in pg_dumpall.c has one. ORDER BY o.roident would keep the dump stable.

+1

>
> 3) Trivia: trailing space in the section header --
>
>     + fprintf(OPF, "--\n-- Replication Origins \n--\n\n");
>
> and a stray blank line at the end of replorigin_create_with_id():
>
>     +    false /* WAL log */);
>     + }
>     +
>     +}
>
> Also, per my earlier mail, the >= 905 / >= 90500 gates can come out now that
> 14d8418083 raised the pg_upgrade floor to v10.
>
> Thanks,
> Rui

RE: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-07-08T06:21:07Z

Dear Shveta,

> For this reason, my inclination is to revert to the previous
> implementation, where we compare old_cluster.nsubs with
> max_active_replication_origins. But I'd like to hear what others think
> before we make this change.

But if origin created by users (or any extensions) have already been tracking the
Remote_lsn, they should be taken into account. If there are three enabled
subscriptions, two active user-created origins but the max_active_origin is set
to three on the new node, the upgrading should fail.
 
I prefer to preserve the current behavior of the patch, which compares
max_active_origin and total number of origins. It might cause the false-positive,
but even now it can happen. E.g., if there was a subscription which has never
been enabled.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

shveta malik <shveta.malik@gmail.com> — 2026-07-08T06:27:37Z

On Wed, Jul 8, 2026 at 11:51 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shveta,
>
> > For this reason, my inclination is to revert to the previous
> > implementation, where we compare old_cluster.nsubs with
> > max_active_replication_origins. But I'd like to hear what others think
> > before we make this change.
>
> But if origin created by users (or any extensions) have already been tracking the
> Remote_lsn, they should be taken into account. If there are three enabled
> subscriptions, two active user-created origins but the max_active_origin is set
> to three on the new node, the upgrading should fail.
>
> I prefer to preserve the current behavior of the patch, which compares
> max_active_origin and total number of origins. It might cause the false-positive,
> but even now it can happen. E.g., if there was a subscription which has never
> been enabled.
>

Okay, I missed this point earlier. I agree, let's retain the current
behaviour itself.

thanks
Shveta

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Amit Kapila <amit.kapila16@gmail.com> — 2026-07-08T09:09:51Z

On Wed, Jul 8, 2026 at 11:51 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > For this reason, my inclination is to revert to the previous
> > implementation, where we compare old_cluster.nsubs with
> > max_active_replication_origins. But I'd like to hear what others think
> > before we make this change.
>
> But if origin created by users (or any extensions) have already been tracking the
> Remote_lsn, they should be taken into account. If there are three enabled
> subscriptions, two active user-created origins but the max_active_origin is set
> to three on the new node, the upgrading should fail.
>

Fair point but isn't it sufficient to check the tracked_origins rather
than all origins? In docs of max_active_replication_origins, we say:
"Setting it to a lower value than the current number of tracked
replication origins (reflected in pg_replication_origin_status) will
prevent the server from starting." which suggests here we should have
a tight check of tracked_origins against
max_active_replication_origins.

> I prefer to preserve the current behavior of the patch, which compares
> max_active_origin and total number of origins. It might cause the false-positive,
> but even now it can happen. E.g., if there was a subscription which has never
> been enabled.
>

I think it is possible that non-tracked origins (e.g for subscriptions
that get enabled only after upgrade) could lead to ERROR after upgrade
and starting the server but users can always increase
max_active_replication_origins to accommodate that which she anyway
needs to do even before upgrade.

-- 
With Regards,
Amit Kapila.

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Amit Kapila <amit.kapila16@gmail.com> — 2026-07-08T10:02:35Z

On Tue, Jun 16, 2026 at 2:50 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, Jun 15, 2026 at 9:31 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
>>
>> Separately, I don't think we actually need to preserve the
>> subscription OID to meet the goal here. pg_commit_ts stores the
>> numeric roident, not the origin name or the sub OID, and conflict
>> detection compares roidents. So the invariant we must keep is "each
>> subscription owns the same roident after upgrade", which doesn't
>> depend on the name being stable.
>>
>>
>
> Replication origins are global objects and are therefore dumped by pg_dumpall, while subscriptions are database-specific and are dumped by pg_dump. In pg_upgrade, pg_dumpall runs before pg_dump.
>
> The complication if subscription OID is not preserved is that subscription-associated origins derive their name from the subscription's OID. If the subscription OID is not preserved across upgrades, the origin name on the new cluster will differ from the old one, making it impossible to restore the origin independently. For these origins, creation must happen after CreateSubscription has established the new OID and not upfront in pg_dumpall.
>
> Non-subscription origins have no such dependency and continue to be created separately as before. The subscription code also cannot be left untouched: even once the subscription is created, rather than origin being created inside CreateSubscription, its associated origin must still be explicitly created via binary_upgrade_create_replication_origin to ensure the roident matches the original node, which means CreateSubscription needs to be modified anyways. So, there is an advantage of reducing code if subscription OID is preserved and all origins keep the same name as the old node.
>

I think here the key is not reducing the code foot print in
CreateSubscription rather the non-subscription origins anyway needs to
be created separately by pg_dumpall as they don't have any binding
with subscription. So, if we don't preserve sub OIDs, we will never be
able to match the names of origins created with pg_dumpall and then
later used by subscriptions. I feel this reasoning should be captured
in the commit message.

-- 
With Regards,
Amit Kapila.

RE: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2026-07-08T10:07:03Z

Dear Amit,

> Fair point but isn't it sufficient to check the tracked_origins rather
> than all origins? In docs of max_active_replication_origins, we say:
> "Setting it to a lower value than the current number of tracked
> replication origins (reflected in pg_replication_origin_status) will
> prevent the server from starting." which suggests here we should have
> a tight check of tracked_origins against
> max_active_replication_origins.

Basically it sounds good, but it can slightly change the behavior if the old node
has some subscriptions which have never been enabled. Till now, we just counted
the number of subscriptions, thus we took into account them [1]. But after the
patch they can be ignored; IIUC pg_upgrade can succeed in the case.

If the change is OK, we can proceed with your (and Shlok-san's) idea.

[1]: reproducer is below.
1. initialized old node with max_active_replication_origins=1
2. created two subscriptions with (connect=false)
3. initialized new node with max_active_replication_origins=1
4. ran pg_upgrade and it did error-out

```
$ pg_upgrade -b /usr/local/pgsql/bin/ -d old/ -D new/ -U postgres
Performing Consistency Checks
-----------------------------
...
Checking new cluster configuration for subscriptions          
"max_active_replication_origins" (1) must be greater than or equal to the number of subscriptions (2) in the old cluster
Failure, exiting
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Amit Kapila <amit.kapila16@gmail.com> — 2026-07-08T10:42:11Z

On Wed, Jul 8, 2026 at 3:37 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > Fair point but isn't it sufficient to check the tracked_origins rather
> > than all origins? In docs of max_active_replication_origins, we say:
> > "Setting it to a lower value than the current number of tracked
> > replication origins (reflected in pg_replication_origin_status) will
> > prevent the server from starting." which suggests here we should have
> > a tight check of tracked_origins against
> > max_active_replication_origins.
>
> Basically it sounds good, but it can slightly change the behavior if the old node
> has some subscriptions which have never been enabled. Till now, we just counted
> the number of subscriptions, thus we took into account them [1]. But after the
> patch they can be ignored; IIUC pg_upgrade can succeed in the case.
>
> If the change is OK, we can proceed with your (and Shlok-san's) idea.
>
> [1]: reproducer is below.
> 1. initialized old node with max_active_replication_origins=1
> 2. created two subscriptions with (connect=false)
> 3. initialized new node with max_active_replication_origins=1
> 4. ran pg_upgrade and it did error-out
>
> ```
> $ pg_upgrade -b /usr/local/pgsql/bin/ -d old/ -D new/ -U postgres
> Performing Consistency Checks
> -----------------------------
> ...
> Checking new cluster configuration for subscriptions
> "max_active_replication_origins" (1) must be greater than or equal to the number of subscriptions (2) in the old cluster
> Failure, exiting
> ```
>

Yes, I understand this case but I feel the old check is more
restrictive than required. We might want to even back-patch this small
part in future if we see any such user complaint.

-- 
With Regards,
Amit Kapila.