Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Thu, May 7, 2026 at 5:47 PM Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> 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 <zsolt(dot)parragi(at)percona(dot)com> 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