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>
 <CANhcyEVd7eDKGtsrUqwZO5McTG4dsJ+X8OxO31Xgu6sWmo--hw@mail.gmail.com>
 <CAFPTHDbyVt_ggdi+rOP4=jBmgU=DDgYWuLxc4oQ6ux-T0H-ZBg@mail.gmail.com>
In-Reply-To: 
 <CAFPTHDbyVt_ggdi+rOP4=jBmgU=DDgYWuLxc4oQ6ux-T0H-ZBg@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Fri, 19 Jun 2026 13:18:34 +0530
Message-ID: 
 <CANhcyEUhTTzFktkyxpsLeq1dQWVRNNN-=ZS6HC_Jnb1srj4-BQ@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/CANhcyEUhTTzFktkyxpsLeq1dQWVRNNN-%3DZS6HC_Jnb1srj4-BQ%40mail.gmail.com>
Precedence: bulk

On Wed, 17 Jun 2026 at 14:49, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, Jun 5, 2026 at 8:10=E2=80=AFPM vignesh C <vignesh21@gmail.com> wr=
ote:
> >
> > Few comments:
> > 1) pg_dumpall dumps all replication origins, not just subscription
> > origins and recreates every origin:
> > SELECT o.*, os.remote_lsn
> > FROM pg_replication_origin o
> > LEFT JOIN pg_replication_origin_status os
> >   ON o.roident =3D os.local_id
> >
> > Is pg_upgrade expected to preserve user-created origins?
> > If so, you can mention this in the commit message
> >
>
> I explicitly state that all replication origins are migrated in the
> commit message, nothing specific about subscription origins.
>
> > 2) Should the table lock be released here, if so should we mention
> > comments similar to how it is mentioned in
> > binary_upgarde_add_sub_rel_state:
> > + /* Restore the remote_lsn if provided, while still holding the lock *=
/
> > + if (!PG_ARGISNULL(2))
> > + {
> > + XLogRecPtr remote_commit =3D PG_GETARG_LSN(2);
> >
> > - ReplicationSlotRelease();
> > + replorigin_advance(node, remote_commit, InvalidXLogRecPtr,
> > +    false /* backward */,
> > +    false /* WAL log */);
> > + }
> > +
> > + table_close(rel, RowExclusiveLock);
> >
>
> Is that relevant when creating a new row? or only when updating an existi=
ng one?
>
> > 3) We can use new style of ereport to exclude '(' before errcode:
> > 3.a)
> > + 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)));
> >
> > 3.b) Similarly here:
> > + ereport(ERROR,
> > + (errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists", node_oid)));
> >
> > 3.c) There are many others, you can search other ereport in the patch
> > and change it too.
> >
>
> Done.
>
> > 4) Since only one column will be returned here, can we use
> > "PQgetvalue(res, 0, 0) and remove PQfnumber:
> > + res =3D executeQueryOrDie(conn,
> > + "SELECT count(*) AS nrepl_origins "
> > + "FROM pg_catalog.pg_replication_origin");
> > + i_nrepl_origins =3D PQfnumber(res, "nrepl_origins");
> > + cluster->nrepl_origins =3D atoi(PQgetvalue(res, 0, i_nrepl_origins));
> > + PQclear(res);
> >
>
> Done.
>
> > 5) replorigin_create also has some duplicate code like below, will it
> > be possible to have a common function so that both of them can use?
> > /* 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)));
> > +
> > + memset(&nulls, 0, sizeof(nulls));
> > + memset(&values, 0, sizeof(values));
> > +
> > + values[Anum_pg_replication_origin_roident - 1] =3D ObjectIdGetDatum(n=
ode);
> > + values[Anum_pg_replication_origin_roname - 1] =3D roname_d;
> > +
> > + tuple =3D heap_form_tuple(RelationGetDescr(rel), values, nulls);
> > + CatalogTupleInsert(rel, tuple);
> > + heap_freetuple(tuple);
> > + CommandCounterIncrement();
> >
>
> I have created a new function replorigin_create_with_id() which has
> some common code and called that from both replorigin_create() and
> binary_upgrade_create_replication_origin()
>
>
> On Tue, Jun 9, 2026 at 3:06=E2=80=AFAM Shlok Kyal <shlok.kyal.oss@gmail.c=
om> wrote:
> >
> > 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" exp=
ected?
> >
>
> I have modified this as option 3 suggested by Shveta and Amit and
> removed the PG17 restriction for migrating replication origins, now it
> does it for all versions that have replication origins.
>
> > 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?
>
> Done
>
>
> On Tue, Jun 9, 2026 at 2:13=E2=80=AFPM shveta malik <shveta.malik@gmail.c=
om> wrote:
> >
> >
> > 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);
> >
>
> I've restructured the code now, its a bit tough to do this now.
>
> > 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)
> >
>
> I've removed the version check for PG17 now.
>
> > 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)));
> >
>
> With the restructured code, its not easy to do this now.
>
> >
> > 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.
> >
>
> Done.
>
>
> On Tue, Jun 9, 2026 at 9:32=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gmail.c=
om> wrote:
> >
> > >
> > 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?
> >
>
> Updated.
>
> I have addressed the comments in v8 (attached). I've also now taken
> out the check for PG17 on old cluster and now all clusters which have
> replication origins are migrated.

Hi Ajin,

Thanks for providing the updated patch, I reviewed the patch and I
have some comments:

1. patch is not applying on HEAD and needs a rebase.

2. New line after 'check_new_cluster_replication_origins' is not required:
+ check_new_cluster_replication_origins();
+

3. pg_dumpall with option '--roles-only' also dumps replication origins
eg:
./pg_dumpall -p 9000 --binary-upgrade --roles-only > op.sql
output contains:
-- For binary upgrade, must preserve replication origin roident and remote_=
lsn
SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.=
oid,
'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);

I think in this case we should not dump the replication origins as we
explicitly gave the option of '--roles-only'

4. pg_dumpall with --globals-only option also dumps replication origins.
eg:
./pg_dumpall -p 9000 --binary-upgrade --globals-only> op.sql and
output contains:
-- For binary upgrade, must preserve replication origin roident and remote_=
lsn
SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.=
oid,
'pg_16387'::pg_catalog.text, '0/00000000'::pg_catalog.pg_lsn);

Is this behaviour intentional? Other logical replication objects
(e.g., publications and subscriptions) are not included in pg_dumpall
--globals-only, so is it okay to dump replication origin?

Thanks,
Shlok Kyal