public inbox for [email protected]  

help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Ajin Cherian <[email protected]>
Cc: Shlok Kyal <[email protected]>
Cc: Zsolt Parragi <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
Date: Tue, 9 Jun 2026 09:43:40 +0530
Message-ID: <CAJpy0uC+OfSSX1denhfi-S-CNChof_au0ACNnLj+kqAvTHNwVQ@mail.gmail.com> (raw)
In-Reply-To: <CAFPTHDavPyxsWjK0cRO3yOaP4u8FGYrOJuXJB-4wzneAY3H3Ug@mail.gmail.com>
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>

On Thu, May 28, 2026 at 8:18 AM Ajin Cherian <[email protected]> wrote:
>
>
>
>> 3)
>>
>> +
>> + /* Dump replication origins */
>> + if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
>> + dumpReplicationOrigins(conn);
>>
>> why the check is for PG17 specifically?
>>
>
> In PG17, we started migrating pg_subscription_rel and the remote LSN during 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 subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created.
>

Okay, please add a comment in code for this.

Please find a few comments on v007:

1)

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

view thread (45+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
  In-Reply-To: <CAJpy0uC+OfSSX1denhfi-S-CNChof_au0ACNnLj+kqAvTHNwVQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox