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>
 <CAHWVJhGoi+2bzZDNxtnB8Xc+DuFKB=YvqC0Xoc_HfzcgHAwMUg@mail.gmail.com>
 <CAFPTHDbWKQjv4QfHaNiXosZ1TD0ZHc2hFyFFNB2RvLsJeJoH6g@mail.gmail.com>
In-Reply-To: 
 <CAFPTHDbWKQjv4QfHaNiXosZ1TD0ZHc2hFyFFNB2RvLsJeJoH6g@mail.gmail.com>
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Wed, 24 Jun 2026 18:57:44 +0530
Message-ID: 
 <CANhcyEWpLNO-zuSsApvCWL=yUMRwFZ9VK9DnSp6Bd93HSRC6fg@mail.gmail.com>
Subject: Re: [PATCH] Preserve replication origin OIDs in pg_upgrade
To: Ajin Cherian <itsajin@gmail.com>
Cc: Rui Zhao <zhaorui126@gmail.com>, 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/CANhcyEWpLNO-zuSsApvCWL%3DyUMRwFZ9VK9DnSp6Bd93HSRC6fg%40mail.gmail.com>
Precedence: bulk

On Tue, 23 Jun 2026 at 17:20, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Wed, Jun 17, 2026 at 8:59=E2=80=AFPM Nisha Moond <nisha.moond412@gmail=
.com> wrote:
> >
> > Hi Ajin, Thanks for the patches.
> > I have started reviewing the patches. Please find a few initial
> > comments below (from v7 review, still applicable to v8):
> >
> > 1) Patch-002: pg_dump.c
> > + if (dopt->binary_upgrade && subinfo->subenabled &&
> > fout->remoteVersion >=3D 170000)
> >   {
> > - if (subinfo->suboriginremotelsn)
> > - {
> > - /*
> > - * Preserve the remote_lsn for the subscriber's replication
> >
> > I could not find any usage of subinfo[i].suboriginremotelsn anymore.
> > It seems to be dead code now. Can we remove it?
>
> Removed it.
>
>
> > ~~~
> >
> > 2) Patch-002 : binary_upgrade_create_replication_origin()
> > - PG_RETURN_VOID();
> > + node_oid =3D PG_GETARG_OID(0);
> > +
> > + if (node_oid =3D=3D InvalidOid || node_oid > PG_UINT16_MAX)
> > + ereport(ERROR,
> > + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("replication origin ID %u is out of range", node_oid));
> > +
> >
> > The above range check allows PG_UINT16_MAX, but that value is not part
> > of the valid replication origin ID range.
> >
> > In origin.h:
> >   #define DoNotReplicateId PG_UINT16_MAX
> >
> > And we already ensure "ReplOriginId !=3D DoNotReplicateId" everywhere
> > else. Would something like the below be more appropriate?
> >   if (node_oid =3D=3D InvalidReplOriginId || node_oid >=3D DoNotReplica=
teId)
> >     ereport(ERROR, ...
> > ~~~
>
> Done.
>
>
> >
> > 3) I think we should add some additional details in
> > logical-replication.sgml, especially under <title>Prepare for
> > Subscriber Upgrades</title>.
> > Some suggestions:
> > 3a) Since all replication origins are now migrated, this:
> >   "...greater than or equal to the number of subscriptions present in
> > the old cluster."
> > could become:
> >   "...greater than or equal to the number of replication origins
> > present in the old cluster."
> >
> > 3b) We should also mention:
> >   The new cluster must contain no replication origins.
> >
>
> Done.
>
>
>
> On Wed, Jun 17, 2026 at 9:42=E2=80=AFPM Nisha Moond <nisha.moond412@gmail=
.com> wrote:
> >
> > 4) I was able to build successfully without all three new header
> > inclusions below, so we may not need them.
> > --- a/src/backend/utils/adt/pg_upgrade_support.c
> > +++ b/src/backend/utils/adt/pg_upgrade_support.c
> > @@ -11,6 +11,7 @@
> > +#include "access/genam.h"
> > ...
> > +#include "utils/fmgroids.h"
> > ...
> > +#include "utils/snapmgr.h"
> > ~~~
> >
>
> Removed.
>
>
> > 5) Race condition in origin.c: replorigin_create()
> > - heap_freetuple(tuple);
> > + table_close(rel, ExclusiveLock);
> > +
> > + replorigin_create_with_id(roident, roname, InvalidXLogRecPtr);
> > +
> >
> > Due to the above refactoring, CREATE SUBSCRIPTION can now fail because
> > of a race between two concurrent CREATE SUBSCRIPTION commands:
> >
> > postgres=3D# create subscription sub2 connection 'dbname=3Dpostgres
> > host=3Dlocalhost port=3D7733' publication pub1;
> > ERROR:  replication origin with ID 2 already exists
> >
> > I think this happens because the table lock is released, allowing
> > another session to create a pg_origin with the next available roident,
> > which replorigin_create_with_id() later attempts to use.
> >
> > Would it make sense to call replorigin_create_with_id() before
> > table_close() so the lock is still held? I tested this and it works,
> > though I'm not fully sure whether self-locking could be a concern
> > here, since replorigin_create_with_id() again acquires a
> > RowExclusiveLock on the same table.
> > ~~~
> >
>
> I have fixed this as suggested by Shveta
>
> ~~~~~~~~~~
>
>
>
> On Thu, Jun 18, 2026 at 7:17=E2=80=AFPM shveta malik <shveta.malik@gmail.=
com> wrote:
> >
> > > I think this happens because the table lock is released, allowing
> > > another session to create a pg_origin with the next available roident=
,
> > > which replorigin_create_with_id() later attempts to use.
> > >
> > > Would it make sense to call replorigin_create_with_id() before
> > > table_close() so the lock is still held? I tested this and it works,
> > > though I'm not fully sure whether self-locking could be a concern
> > > here, since replorigin_create_with_id() again acquires a
> > > RowExclusiveLock on the same table.
> >
> > We can handle this race-condition by moving the table-open to callers
> > rather than internal call replorigin_create_with_id().  But we also
> > need the following changes first:
> >
>
> Yes, did it that way.
>
>
> > 1)
> > replorigin_create_with_id has:
> >
> > + /*
> > + * To avoid needing a TOAST table for pg_replication_origin, we limit
> > + * replication origin names to 512 bytes.  This should be more than en=
ough
> > + * for all practical use.
> > + */
> > + if (strlen(roname) > 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));
> >
> > replorigin_create_with_id() is called by replorigin_create() and
> > binary_upgrade_create_replication_origin().   replorigin_create()
> > already has this check. We should move above check to
> > binary_upgrade_create_replication_origin() and get rid of it from
> > internal function replorigin_create_with_id().
>
> Moved.
>
> >
> > 2)
> >
> > Also the checks:
> > + if (SearchSysCacheExists1(REPLORIGIDENT, ObjectIdGetDatum(roident)))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin with ID %u already exists",
> > +    (Oid) roident));
> > +
> > + if (SearchSysCacheExists1(REPLORIGNAME, roname_d))
> > + ereport(ERROR,
> > + errcode(ERRCODE_DUPLICATE_OBJECT),
> > + errmsg("replication origin \"%s\" already exists", roname));
> >
> > should be moved to caller binary_upgrade_create_replication_origin().
> > IMO, these checks are not needed for replorigin_create(), it alread
> > has this, which should suffice.
> >
> >                 ereport(ERROR,
> >                                 (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED=
),
> >                                  errmsg("could not find free
> > replication origin ID")));
> >
> > ~~
> >
> > Once we correctly place these checks, we can move the 'table_open with
> > lock' to caller (before name and id collision checks) instead of
> > replorigin_create_with_id(), which will take care of race-conditions
> > reported by Nisha.
> >
>
> I have moved the roident check out of replorigin_create_with_id(), but
> kept the roname check there as that is not there in
> replorigin_create()
>
> On Fri, Jun 19, 2026 at 5:48=E2=80=AFPM Shlok Kyal <shlok.kyal.oss@gmail.=
com> wrote:
> > 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 requir=
ed:
> > + 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 rem=
ote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_cata=
log.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'
> >
>
> I have moved the dump_replication_origins under !roles_only and
> !tablespaces_only checks, I think "globals only" is valid for
> replication origins.
>
>
> > 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 rem=
ote_lsn
> > SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_cata=
log.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?
> >
>
> This is expected as publications and subscriptions are database
> specific and not globals, while replication origins are global.
>
>
>
> On Mon, Jun 22, 2026 at 2:22=E2=80=AFAM Rui Zhao <zhaorui126@gmail.com> w=
rote:
> >>
> >> Hi Ajin,
> >>
> >> Thanks for v8. A couple of comments:
> >>
> >> 1) The PG17 gating seems inconsistent between the dump and the check n=
ow.
> >> dumpReplicationOrigins() is called unconditionally:
> >>
> >>     if (binary_upgrade && archDumpFormat =3D=3D archNull)
> >>         dumpReplicationOrigins(conn);
> >>
> >> but get_subscription_info(), which sets nrepl_origins, is still only
> >> called for old clusters >=3D 17 in check_new_cluster():
> >>
> >>     if (GET_MAJOR_VERSION(old_cluster.major_version) >=3D 1700)
> >>         ...
> >>         get_subscription_info(&old_cluster);
> >>
> >> and check_new_cluster_replication_origins() returns early on it:
> >>
> >>     if (old_cluster.nrepl_origins =3D=3D 0)
> >>         return;
> >>
> >> For a < 17 old cluster get_subscription_info() is never called, so
> >> nrepl_origins stays 0 and this returns early. So when such a cluster h=
as a
> >> (user-created) origin, the origin still gets dumped and recreated, whi=
le
> >> the "new cluster must have no origins" and max_active_replication_orig=
ins
> >> checks are skipped, and the upgrade fails during restore instead of at=
 the
> >> check step. nrepl_origins is just a count(*) on pg_replication_origin,=
 so
> >> moving its collection out of the >=3D 1700 block should be enough.
> >>
>
> Yes, good catch. I have separated out the replication origin speciifc
> checks into a different function and moved it oustide the >=3D 1700
> block
>
> >> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Si=
nce
> >> the patch migrates all origins, should we also create a user origin
> >> (pg_replication_origin_create()) and verify its roident/remote_lsn aft=
er
> >> the upgrade? That would also cover the case in (1).
>
>
> Added.
>
> All the above changes have been addressed in patch v9.
>
Thanks for the updated patch. I have reviewed the patch and here are
some my comments:

1. Since we are quering only one column, do we need to use 'i_norigins'?

+void
+get_replication_origin_info(ClusterInfo *cluster)
+{
+ PGconn    *conn;
+ PGresult   *res;
+ int i_norigins;
+
+ conn =3D connectToServer(cluster, "template1");
+ res =3D executeQueryOrDie(conn, "SELECT count(*) AS norigins "
+    "FROM pg_catalog.pg_replication_origin");
+ i_norigins =3D PQfnumber(res, "norigins");
+
+ cluster->nrepl_origins =3D atoi(PQgetvalue(res, 0, i_norigins));
+ PQclear(res);
+
+ PQfinish(conn);
+}
I think we can directly use:
cluster->nrepl_origins =3D atoi(PQgetvalue(res, 0, 0));

Thoughts?

2. In the above code should we also add a check like:
if (PQntuples(res) !=3D 1)
pg_fatal("could not count the number of replication origins");

3. The brackets are not required:
+ if (!tablespaces_only && !roles_only && binary_upgrade)
+ {
+ /* Dump replication origins */
+ dumpReplicationOrigins(conn);
  }
We can move the comment '/* Dump replication origins */' above if condition=
.
It will make it consistent with the existing code.

4. I ran the tests locally and found that the test_decoding test suit
is failing:
not ok 10    - replorigin                                249 ms

diff:
 -- ensure duplicate creations fail
 SELECT pg_replication_origin_create('regress_test_decoding: regression_slo=
t');
-ERROR:  duplicate key value violates unique constraint
"pg_replication_origin_roname_index"
-DETAIL:  Key (roname)=3D(regress_test_decoding: regression_slot) already e=
xists.
+ERROR:  replication origin "regress_test_decoding: regression_slot"
already exists
 -- ensure inactive origin cannot be set as session one if pid is specified
 SELECT pg_replication_origin_session_setup('regress_test_decoding:
regression_slot', -1);
 ERROR:  cannot use PID -1 for inactive replication origin with ID 1

I think this is expected because this patch adds a new function
'replorigin_create_with_id'
which adds a check for the already existing replication origin.
In HEAD we don't have such a check. So I think we should update the
expected output file.

Thanks,
Shlok Kyal