Thread

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