A few more minor comments: binary_upgrade_replorigin_advance seems like dead code in the patch now, it has no callers since the patch removes its only use.
+ ReplOriginId node; .... + node = PG_GETARG_OID(0); ReplOriginId is uint16, this silently truncates it. Since this is a generic callable function shouldn't there be at least a check about it? Also, seems like the same function (binary_upgrade_create_replication_origin) locks-unlocks-locks ReplicationOriginRelationId, that doesn't seem the best approach with a single-use helper function? (first in replorigin_create_with_reploriginid, then binary_upgrade_create_replication_origin reaquires it) + if (PQntuples(res) > 0 && archDumpFormat == archNull) + fprintf(OPF, "--\n-- Replication Origins \n--\n\n"); The caller also checks the format, it is redundant. + /* Get replication origins in current database. */ + appendPQExpBufferStr(buf, Isn't pg_replication_origin a shared catalog?
