public inbox for [email protected]  

help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Ajin Cherian <[email protected]>
Cc: Rui Zhao <[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: Wed, 24 Jun 2026 10:39:00 +0530
Message-ID: <CAJpy0uCCLDN9QHHdSwxaVcVwTid97USt3xAbY+HyzLgQOWW3rg@mail.gmail.com> (raw)
In-Reply-To: <CAFPTHDbWKQjv4QfHaNiXosZ1TD0ZHc2hFyFFNB2RvLsJeJoH6g@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>
	<CANhcyEVFSutXXdt_0XMCD37NE7Yd7ROdDVEE06D8YVyPNEBFHg@mail.gmail.com>
	<CANhcyEVd7eDKGtsrUqwZO5McTG4dsJ+X8OxO31Xgu6sWmo--hw@mail.gmail.com>
	<CAFPTHDbyVt_ggdi+rOP4=jBmgU=DDgYWuLxc4oQ6ux-T0H-ZBg@mail.gmail.com>
	<CAHWVJhGoi+2bzZDNxtnB8Xc+DuFKB=YvqC0Xoc_HfzcgHAwMUg@mail.gmail.com>
	<CAFPTHDbWKQjv4QfHaNiXosZ1TD0ZHc2hFyFFNB2RvLsJeJoH6g@mail.gmail.com>

On Tue, Jun 23, 2026 at 5:20 PM Ajin Cherian <[email protected]> wrote:
>
> All the above changes have been addressed in patch v9.
>

Thanks for the patches. A few ocmments:

1)

+ * In binary-upgrade mode, skip origin creation here. This is required to
+ * preserve the roident from the old cluster for this subscription.

subscription --> subscription's origin

2)
replorigin_create_with_id():

+ if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin \"%s\" already exists", roname));

Can this ever happen? IIUC, since new-cluster should not have any
origin (as per the check introduced) and old cluster can not have
duplicate names, we should not hit this error. Do we want to keep it
as sanity check? If so, can we put a comment atop it.

3)
replorigin_create_with_id:

+ if (remote_lsn != InvalidXLogRecPtr)
+ replorigin_advance(roident, remote_lsn, InvalidXLogRecPtr,
+    false /* backward */,
+    false /* WAL log */);

We do not want to call 'advance' from regular 'replorigin_create'
flow. It is only for binary-Upgrade flow. Do you think we should have
a sanity check here?

if (remote_lsn != InvalidXLogRecPtr)
{
    Assert(IsBinaryUpgrade)
    replorigin_advance(..)
}

4)
replorigin_create:

+ found = true;

+ if (!found)
  ereport(ERROR,
  (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
  errmsg("could not find free replication origin ID")));

Do you think we can make use of 'collides' itself here instead of
adding a new 'found' variable. We can move 'collides' outside of loop
and use it to emit above error.

5)
binary_upgrade_create_replication_origin:

+ node = (ReplOriginId) node_oid;
+
+ if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(node)))
+ ereport(ERROR,
+ errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("replication origin with ID %u already exists",
+    (Oid) node));

How can we hit this? Same comment as #2.

6)
-check_new_cluster_subscription_configuration(void)
+check_new_cluster_replication_origins(void)

- /* Quick return if there are no subscriptions to be migrated. */
- if (old_cluster.nsubs == 0)
- return;

- if (old_cluster.nsubs > max_active_replication_origins)

With these changes, do we even need to capture old-cluster's 'nsubs'
in get_subscription_info(). I do not see any other usage of nsubs. Let
me know if I am missing any case.

thanks
Shveta

view thread (44+ 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], [email protected]
  Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
  In-Reply-To: <CAJpy0uCCLDN9QHHdSwxaVcVwTid97USt3xAbY+HyzLgQOWW3rg@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