Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Mon, 18 May 2026 at 16:13, Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > Rebased the patch as it was no longer applying.
> >
> Hi Ajin,
>
> I have started reviewing the patch. Here is my comment for v6-0002 patch:
>
> Suppose we have a replication setup: publisher -> subscriber
> and we are upgrading subscriber to subscriber_new.
> And if initially 'subscriber_new' has a replication origin, upgrading
> the cluster can error out.
>
> Example:
> We set up a logical replication between publisher node and subscriber node.
>
> On subscriber node:
> postgres=# SELECT * FROM pg_replication_origin;
>  roident |  roname
> ---------+----------
>        1 | pg_16393
> (1 row)
>
> And initially subscriber_new has a replication origin:
> postgres=# select pg_replication_origin_create('myname');
>  pg_replication_origin_create
> ------------------------------
>                             1
> (1 row)
>
> postgres=# SELECT * FROM pg_replication_origin;
>  roident | roname
> ---------+--------
>        1 | myname
> (1 row)
>
> Now, if we run pg_upgrade to upgrade subscriber node to subscriber_new
> node, we get an error:
> ```
> SELECT pg_catalog.binary_upgrade_create_replication_origin('1'::pg_catalog.oid,
> 'pg_16393'::pg_catalog.name, '0/01743078'::pg_catalog.pg_lsn);
> psql:subscriber_new/pg_upgrade_output.d/20260522T140312.807/dump/pg_upgrade_dump_globals.sql:37:
> ERROR:  replication origin with ID 1 already exists
> ```
>
> This error occurs in "Performing Upgrade" stage. Should we add a check
> in the "Performing Consistency Checks" stage so that we don't need to
> re-initdb the new cluster to perform the upgrade?
> Maybe we can add a check similar to
> check_new_cluster_replication_slots(), where pg_upgrade errors out if
> the new cluster already contains replication origins. Thoughts?

+1. I had the same thought while reviewing the patch today. We should
have it unless there is a reason we have avoided it??

Few trivial comments:

1)

+#include "access/skey.h"
+#include "catalog/indexing.h"

pg_upgrade_support.c compiles without above.

2)
+ Assert(!OidIsValid(rel->rd_rel->reltoastrelid));

Is there a reason for this sanity check? I generally do not see a
Null-Toast table sanity check after every table_open.

3)

+
+ /* Dump replication origins */
+ if (server_version >= 170000 && binary_upgrade && archDumpFormat == archNull)
+ dumpReplicationOrigins(conn);

why the check is for PG17 specifically?

thanks
Shveta