On Thu, May 7, 2026 at 5:47 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Dear Ajin, > > Thanks for updating the patch. Let me share my two high-level comments. > > 1. > Can you clarify the policy for backward compatibility? In other words, should > we > preserve subscription OIDs and migrate replication origins from PG19- > instances? > Similar commits 9a17be1 and 29d0a77 did not allow migrating objects from > released > versions.
I thought about this and after discussions with others decided that we should support migrating replication_origins from PG17 onwards. Prior to that pg_subscription_rel and remote LSN were not updated as part of migration. It's important that we support migration of versions prior to PG_19, otherwise we will end up skipping creating replication origins as part of CREATE SUBSCRIPTION in the new cluster and subscriptions will be without replication origins. So, I have updated the check to migrate replication origins as long as the old cluster is PG17 or greater. > > 2. > I found that other objects use global variables like binary_upgrade_next_xxx > to > specify the next OID. Can we follow the manner even for replication origins? > Or it's not a good approach because of some reasons? > This naming convention is followed for preserving subscription OIDs but since for replication origins we are creating each replication origin with the same roname and roident, I don't think the same situation applies here. > BTW, cfbot got angry with your patch. Yes, fixed that. On Fri, May 8, 2026 at 7:06 AM Zsolt Parragi <[email protected]> wrote: > > Hello! > > + appendPQExpBuffer(buf, > + "SELECT > pg_catalog.binary_upgrade_create_replication_origin('%u'::pg_catalog.oid, > '%s'::pg_catalog.name", > + roident, roname); > + > + if (!PQgetisnull(res, i, i_remotelsn)) > + { > + appendPQExpBuffer(buf, ", '%s'::pg_catalog.pg_lsn", > + PQgetvalue(res, i, i_remotelsn)); > + } > + else > > Isn't some string escaping missing from this? It doesn't seem like the > most likely SQL injection target, but could still cause surprises. > Could be even verified by a test case using > pg_replication_origin_create('O''Brien_replica') > Fixed. > - if (old_cluster.nsubs > max_active_replication_origins) > + if (old_cluster.nrepl_origins > max_active_replication_origins) > pg_fatal("\"max_active_replication_origins\" (%d) must be greater > than or equal to the number of " > "subscriptions (%d) on the old cluster", > - max_active_replication_origins, old_cluster.nsubs); > + max_active_replication_origins, old_cluster.nrepl_origins); > > This error message could be misleading now, it's not the number of > subscriptions but the number of replication origins. > Fixed. > + rel = table_open(ReplicationOriginRelationId, ExclusiveLock); > > Do we need an ExclusiveLock, couldn't this instead use a > RowExclusiveLock? In the unlikely situation that another session > inserts the same record, the unique index should be able catch it? > Fixed. > > +#include "access/xlogdefs.h" > > This seems to be unnecessary? > It is required. > - appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn); > + if (fout->remoteVersion < 190000 && subinfo->suboriginremotelsn) > + { > > subinfo->suboriginremotelsn is already checked by the outer if Fixed. Attaching version 4 with the fixes. regards, Ajin Cherian Fujitsu Australia
v4-0001-Preserve-subscription-OIDs-during-pg_upgrade.patch
Description: Binary data
v4-0002-Preserve-replication-origin-OIDs-during-pg_upgrad.patch
Description: Binary data
