MIME-Version: 1.0
References: 
 <CAFPTHDa5aP8ixTZBygGAe48E1N=DvuPzcXikw7bNKixLMN3pVg@mail.gmail.com>
 <OS9PR01MB12149E188221BED862E4BE9E0F5342@OS9PR01MB12149.jpnprd01.prod.outlook.com>
 <CALDaNm2-uwpbJ8fnrssp+hORvOutsqRoZAsa05xVVzXe5Bt3bw@mail.gmail.com>
 <CAFPTHDaNLRqPAda1RUDVfSDH7eLTaONv0UEmc9H5sdMW1Li2Bg@mail.gmail.com>
 <CAN4CZFOb3urMsLPsEyVrYR7-7yA+BC5kDgQQd0nAQ8xj2zyRcA@mail.gmail.com>
 <CAFPTHDYqpuZ6o2-HuCJDYqJ7GY3+zV+Xo-gT7PAgi4Bkz+oTxw@mail.gmail.com>
 <CAN4CZFN4oLdxLwUXHUPV-5mFmK+4dcnppP00fV3i4qmMYCAkGA@mail.gmail.com>
 <CAFPTHDbBiTfjYkQwYTNA2a4LRp8Sp7_zB59fmV0R7977ztxgmg@mail.gmail.com>
 <CAFPTHDaftSwzGTGbFEw8rwDBsub0XqcDm1wxQcquj-Y3PC2qrg@mail.gmail.com>
 <CANhcyEVercsNbgC5CTkMNzh_w_Jv1uK7RQg9vEvAeNQhoBHCKQ@mail.gmail.com>
 <CAJpy0uAXynzWr4w_9GkJC3i=LL9WsRCZYVWAV0PgKmgs8riWzg@mail.gmail.com>
In-Reply-To: 
 <CAJpy0uAXynzWr4w_9GkJC3i=LL9WsRCZYVWAV0PgKmgs8riWzg@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Mon, 25 May 2026 12:43:27 +0530
Message-ID: 
 <CAJpy0uD3CtFbGyTnCknxoMLnPBco642KUM0zwz8JEPjxw7iK4Q@mail.gmail.com>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
To: Shlok Kyal <shlok.kyal.oss@gmail.com>, Ajin Cherian <itsajin@gmail.com>
Cc: Zsolt Parragi <zsolt.parragi@percona.com>,
 vignesh C <vignesh21@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 shveta malik <shvetamalik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uD3CtFbGyTnCknxoMLnPBco642KUM0zwz8JEPjxw7iK4Q%40mail.gmail.com>
Precedence: bulk

On Fri, May 22, 2026 at 3:57=E2=80=AFPM shveta malik <shveta.malik@gmail.co=
m> wrote:
>
> On Fri, May 22, 2026 at 3:16=E2=80=AFPM 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 patc=
h:
> >
> > 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 n=
ode.
> >
> > On subscriber node:
> > postgres=3D# SELECT * FROM pg_replication_origin;
> >  roident |  roname
> > ---------+----------
> >        1 | pg_16393
> > (1 row)
> >
> > And initially subscriber_new has a replication origin:
> > postgres=3D# select pg_replication_origin_create('myname');
> >  pg_replication_origin_create
> > ------------------------------
> >                             1
> > (1 row)
> >
> > postgres=3D# 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_cata=
log.oid,
> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upg=
rade_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 >=3D 170000 && binary_upgrade && archDumpFormat =3D=
=3D archNull)
> + dumpReplicationOrigins(conn);
>
> why the check is for PG17 specifically?
>

One issue in 002:

binary_upgrade_create_replication_origin() has this:

+ originname =3D PG_GETARG_NAME(1);
+
+ roname_d =3D 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=3D# SELECT
pg_replication_origin_create('this_is_a_very_long_replication_origin_name_t=
hat_exceeds_the_limit_of_64');
 pg_replication_origin_create
------------------------------
                            1
postgres=3D# select * from pg_replication_origin;
 roident |                                roname
---------+-----------------------------------------------------------------=
-----
       1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_lim=
it_of_64


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

thanks
Shveta