Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

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')

- 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.

+ 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?

+#include "access/xlogdefs.h"

This seems to be unnecessary?

- appendPQExpBuffer(query, ", '%s');\n", subinfo->suboriginremotelsn);
+ if (fout->remoteVersion < 190000 && subinfo->suboriginremotelsn)
+ {

subinfo->suboriginremotelsn is already checked by the outer if