Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

Hi Ajin,

Thanks for v9, and for addressing both my earlier comments. I also went
through the rest of v9 -- the other reviewers' comments and the
corresponding fixes (including the replorigin_create() locking rework) look
good to me; I didn't find any issues there.

A follow-up on the version gating, though: I think it now moved a bit too
far. get_replication_origin_info() is called unconditionally (outside the
>= 1700 block):

/* Get replication origin information */
get_replication_origin_info(&old_cluster);

and it runs "SELECT count(*) FROM pg_catalog.pg_replication_origin".
pg_dumpall's dumpReplicationOrigins() queries the same catalog with no
version guard either. But pg_replication_origin only exists from 9.5
(commit 5aa2350426), while pg_upgrade still accepts old clusters back to
9.2:

if (GET_MAJOR_VERSION(old_cluster.major_version) < 902)
pg_fatal("This utility can only upgrade from PostgreSQL version %s
and later.", "9.2");

So upgrading from 9.2/9.3/9.4 now fails: the query hits a non-existent
catalog and pg_fatal()s (and dumpReplicationOrigins would too).
It should be gated on the version where origins were introduced, e.g.

if (GET_MAJOR_VERSION(old_cluster.major_version) >= 905)
get_replication_origin_info(&old_cluster);

with the matching server_version >= 90500 check on the pg_dumpall side.
That keeps the < 17 case I flagged working while not breaking 9.2-9.4
(which have no origins to migrate anyway).

One nit while here: there is an extra blank line after the
check_new_cluster_replication_origins() call in check_new_cluster().

Thanks,
Rui

Ajin Cherian <itsajin(at)gmail(dot)com> 于2026年6月23日周二 19:50写道：
>
> On Wed, Jun 17, 2026 at 8:59 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)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 >= 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 = PG_GETARG_OID(0);
> > +
> > + if (node_oid == 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 != DoNotReplicateId" everywhere
> > else. Would something like the below be more appropriate?
> > if (node_oid == InvalidReplOriginId || node_oid >= DoNotReplicateId)
> > 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 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)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=# create subscription sub2 connection 'dbname=postgres
> > host=localhost port=7733' 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 PM shveta malik <shveta(dot)malik(at)gmail(dot)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 enough
> > + * 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 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)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 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'
> >
>
> 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 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?
> >
>
> 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 AM Rui Zhao <zhaorui126(at)gmail(dot)com> wrote:
> >>
> >> Hi Ajin,
> >>
> >> Thanks for v8. A couple of comments:
> >>
> >> 1) The PG17 gating seems inconsistent between the dump and the check now.
> >> dumpReplicationOrigins() is called unconditionally:
> >>
> >> if (binary_upgrade && archDumpFormat == archNull)
> >> dumpReplicationOrigins(conn);
> >>
> >> but get_subscription_info(), which sets nrepl_origins, is still only
> >> called for old clusters >= 17 in check_new_cluster():
> >>
> >> if (GET_MAJOR_VERSION(old_cluster.major_version) >= 1700)
> >> ...
> >> get_subscription_info(&old_cluster);
> >>
> >> and check_new_cluster_replication_origins() returns early on it:
> >>
> >> if (old_cluster.nrepl_origins == 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 has a
> >> (user-created) origin, the origin still gets dumped and recreated, while
> >> the "new cluster must have no origins" and max_active_replication_origins
> >> 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 >= 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 >= 1700
> block
>
> >> 2) 004_subscription.pl only checks subscription origins (pg_<oid>). Since
> >> the patch migrates all origins, should we also create a user origin
> >> (pg_replication_origin_create()) and verify its roident/remote_lsn after
> >> the upgrade? That would also cover the case in (1).
>
>
> Added.
>
> All the above changes have been addressed in patch v9.
>
> regards,
> Ajin Cherian
> Fujitsu Australia