On Fri, May 22, 2026 at 8:27 PM shveta malik <[email protected]> wrote:
> 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?? > Fixed this. Added a new check for replication origins and if the new cluster has any existing replication origins, then the check will fail. > > Few trivial comments: > > 1) > > +#include "access/skey.h" > +#include "catalog/indexing.h" > > pg_upgrade_support.c compiles without above. > > Removed. > 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. > > Removed. > 3) > > + > + /* Dump replication origins */ > + if (server_version >= 170000 && binary_upgrade && archDumpFormat == > archNull) > + dumpReplicationOrigins(conn); > > why the check is for PG17 specifically? > > In PG17, we started migrating pg_subscription_rel and the remote LSN during upgrades; prior to that, these were not migrated. Given that change, it also makes sense to migrate replication origins from them. Otherwise, when upgrading from PG17 to a later version, you could end up with a subscription where pg_subscription_rel and the remote LSN are migrated, but the corresponding replication origin is not created. On Mon, May 25, 2026 at 5:13 PM shveta malik <[email protected]> wrote: > > One issue in 002: > > binary_upgrade_create_replication_origin() has this: > > + originname = PG_GETARG_NAME(1); > + > + roname_d = CStringGetTextDatum(NameStr(*originname)); > + > > We are getting origin-name (text) into Name-type which can not be more > than 64 bytes. So if an origin has name more than 64, it will end up > trimming the name post-upgrade. > > I tried this: > > Old-setup: > postgres=# SELECT > > pg_replication_origin_create('this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64'); > pg_replication_origin_create > ------------------------------ > 1 > postgres=# select * from pg_replication_origin; > roident | roname > > ---------+---------------------------------------------------------------------- > 1 | > this_is_a_very_long_replication_origin_name_that_exceeds_the_limit_of_64 > > > Post-upgrade: name got trimmed to 64 length. > ------------------------- > postgres=# select * from pg_replication_origin; > roident | roname > ---------+----------------------------------------------------------------- > 1 | this_is_a_very_long_replication_origin_name_that_exceeds_the_li > > thanks > Shveta Fixed this. Now binary_upgrade_create_replication_origin handles it similarly to the way pg_replication_origin_create handles the name of the origin. Here's an updated version v7 containing these fixes. regards, Ajin Cherian Fujitsu Australia
v7-0001-Preserve-subscription-OIDs-during-pg_upgrade.patch
Description: Binary data
v7-0002-Preserve-replication-origin-OIDs-during-pg_upgrad.patch
Description: Binary data
