On Fri, May 22, 2026 at 3:16 PM Shlok Kyal <[email protected]> wrote: > > On Mon, 18 May 2026 at 16:13, Ajin Cherian <[email protected]> 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
