[PATCH] Preserve replication origin OIDs in pg_upgrade

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]/messages/by-id/CALDaNm2Wj63VcbB0SY2NECHr1mKM1YSaV1ZydrdQVxyox2O2hg@mail.gmail.com, 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]: /messages/by-id/CALDaNm2Wj63VcbB0SY2NECHr1mKM1YSaV1ZydrdQVxyox2O2hg@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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

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

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

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

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

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

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

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

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?

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

Rebased the patch as it was no longer applying.

regards,
Ajin Cherian
Fujitsu Australia

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

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

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

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

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

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

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