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>
 <CAFPTHDavPyxsWjK0cRO3yOaP4u8FGYrOJuXJB-4wzneAY3H3Ug@mail.gmail.com>
 <CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg@mail.gmail.com>
In-Reply-To: 
 <CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Tue, 9 Jun 2026 17:02:00 +0530
Message-ID: 
 <CANhcyEVd7eDKGtsrUqwZO5McTG4dsJ+X8OxO31Xgu6sWmo--hw@mail.gmail.com>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
To: Ajin Cherian <itsajin@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>,
 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/CANhcyEVd7eDKGtsrUqwZO5McTG4dsJ%2BX8OxO31Xgu6sWmo--hw%40mail.gmail.com>
Precedence: bulk

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=E2=80=AFPM shveta malik <shveta.malik@gmai=
l.com> wrote:
> >>
> >> On Fri, May 22, 2026 at 3:16=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gma=
il.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 p=
atch:
> >> >
> >> > 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, upgradin=
g
> >> > the cluster can error out.
> >> >
> >> > Example:
> >> > We set up a logical replication between publisher node and subscribe=
r node.
> >> >
> >> > 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_n=
ew
> >> > node, we get an error:
> >> > ```
> >> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_c=
atalog.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 che=
ck
> >> > in the "Performing Consistency Checks" stage so that we don't need t=
o
> >> > 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 i=
f
> >> > 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 cl=
uster 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 >=3D 170000 && binary_upgrade && archDumpFormat =
=3D=3D archNull)
> >> + dumpReplicationOrigins(conn);
> >>
> >> why the check is for PG17 specifically?
> >>
> >
> > In PG17, we started migrating pg_subscription_rel and the remote LSN du=
ring upgrades; prior to that, these were not migrated. Given that change, i=
t also makes sense to migrate replication origins from them. Otherwise, whe=
n upgrading from PG17 to a later version, you could end up with a subscript=
ion where pg_subscription_rel and the remote LSN are migrated, but the corr=
esponding replication origin is not created.
> >
> >
> >
> > On Mon, May 25, 2026 at 5:13=E2=80=AFPM shveta malik <shveta.malik@gmai=
l.com> wrote:
> >>
> >>
> >> 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_n=
ame_that_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_th=
e_limit_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_th=
e_li
> >>
> >> thanks
> >> Shveta
> >
> >
> > Fixed this. Now  binary_upgrade_create_replication_origin handles it si=
milarly to the way pg_replication_origin_create handles the name of the ori=
gin.
> >
> > 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 <=3D 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 >=3D 170000 && binary_upgrade &&
> archDumpFormat =3D=3D 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" expec=
ted?
>
> 2.a) Can we replace:
> +   /* Check for OID collision */
> +   ScanKeyInit(&key,
> +               Anum_pg_replication_origin_roident,
> +               BTEqualStrategyNumber, F_OIDEQ,
> +               ObjectIdGetDatum(node));
> +   scan =3D systable_beginscan(rel, ReplicationOriginIdentIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides =3D 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 =3D systable_beginscan(rel, ReplicationOriginNameIndex,
> +                             true /* indexOK */,
> +                             SnapshotSelf,
> +                             1, &key);
> +   collides =3D 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