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: vignesh C <vignesh21@gmail.com>
Date: Fri, 5 Jun 2026 15:39:49 +0530
Message-ID: 
 <CALDaNm1L7++aSjacbFayEvgsj5zjdfa9tc3sGw6N9NVt7DSOKQ@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>,
 Shlok Kyal <shlok.kyal.oss@gmail.com>,
	Zsolt Parragi <zsolt.parragi@percona.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/CALDaNm1L7%2B%2BaSjacbFayEvgsj5zjdfa9tc3sGw6N9NVt7DSOKQ%40mail.gmail.com>
Precedence: bulk

On Thu, 28 May 2026 at 08:18, Ajin Cherian <itsajin@gmail.com> wrote:
>
> 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.

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

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);

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.

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);

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(node)=
;
+ 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();

Regards,
Vignesh