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>
In-Reply-To: 
 <CAFPTHDavPyxsWjK0cRO3yOaP4u8FGYrOJuXJB-4wzneAY3H3Ug@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Mon, 8 Jun 2026 22:36:13 +0530
Message-ID: 
 <CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg@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/CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg%40mail.gmail.com>
Precedence: bulk

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@gmail.=
com> 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 pat=
ch:
>> >
>> > 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=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_cat=
alog.oid,
>> > 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
>> > psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_up=
grade_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 clus=
ter 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 duri=
ng 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 subscriptio=
n where pg_subscription_rel and the remote LSN are migrated, but the corres=
ponding replication origin is not created.
>
>
>
> On Mon, May 25, 2026 at 5:13=E2=80=AFPM shveta malik <shveta.malik@gmail.=
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_nam=
e_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_the_=
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_the_=
li
>>
>> thanks
>> Shveta
>
>
> Fixed this. Now  binary_upgrade_create_replication_origin handles it simi=
larly to the way pg_replication_origin_create handles the name of the origi=
n.
>
> 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" expecte=
d?

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?

Thanks,
Shlok Kyal