Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Thu, 9 Jul 2026 at 17:07, Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> Attaching v11 with all comments addressed.
>

Thanks Ajin. I reviewed the patch. Here are some comment for it:

1. pg_dumpall.c
dumpRoleGUCPrivs(conn);
+
}

+ /* Dump replication origins */
+ if (!tablespaces_only && !roles_only && binary_upgrade)
+ dumpReplicationOrigins(conn);

The blank line below 'dumpRoleGUCPrivs(conn);' is not required

2. Maybe we can initialize the variables during declaration itself:
+ ReplOriginId roident;
+ const char *roname;
+
+ roident = atooid(PQgetvalue(res, i, i_roident));
+ roname = PQgetvalue(res, i, i_roname);

3. We should add a comment on top of this function. Just like other
dumpXXX functions
+static void
+dumpReplicationOrigins(PGconn *conn)

4. In check.c:
+ /*
+ * Compare against the number of active origins in the old cluster not the
+ * total number of origins in pg_replication_origin. An origin can exist

I think we can reword it like:
Compare against the number of active replication origins in the old
cluster, rather than the total number of origins in
pg_replication_origin.

5. In pg_upgrade_support.c:
+#include "utils/syscache.h"

We can remove this include.

Thanks,
Shlok Kyal