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: shveta malik <shveta.malik@gmail.com>
Date: Tue, 9 Jun 2026 09:43:40 +0530
Message-ID: 
 <CAJpy0uC+OfSSX1denhfi-S-CNChof_au0ACNnLj+kqAvTHNwVQ@mail.gmail.com>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
To: Ajin Cherian <itsajin@gmail.com>
Cc: Shlok Kyal <shlok.kyal.oss@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 <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uC%2BOfSSX1denhfi-S-CNChof_au0ACNnLj%2BkqAvTHNwVQ%40mail.gmail.com>
Precedence: bulk

On Thu, May 28, 2026 at 8:18=E2=80=AFAM Ajin Cherian <itsajin@gmail.com> wr=
ote:
>
>
>
>> 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.
>

Okay, please add a comment in code for this.

Please find a few comments on v007:

1)

+ node =3D (ReplOriginId) node_oid;
+ originname =3D 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 =3D 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 =3D PG_GETARG_TEXT_PP(1);
originname =3D text_to_cstring(origin_text);
roname_d =3D 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 =3D=3D 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