Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Wed, Jun 17, 2026 at 8:59 PM 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 >= 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.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=# 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.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 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.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 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@gmail.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