Re: [PATCH] Preserve replication origin OIDs in pg_upgrade

On Wed, Aug 19, 2026 at 11:13 AM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
>
> I have incorporated your comments and your changes in a new patch v16.
>

Please find a few minor comments:

1)
+ if (!PQgetisnull(res, i, i_remotelsn))
+ {
+ appendPQExpBufferStr(buf, ", ");
+ appendStringLiteralConn(buf, PQgetvalue(res, i, i_remotelsn), conn);
+ appendPQExpBufferStr(buf, "::pg_catalog.pg_lsn");
+ }
+ else
+ appendPQExpBufferStr(buf, ", NULL");

It is recommended to typecast even NULL to datatype to remove any
ambiguity in function signature matching (for future cases). IIUC,
Null is treated as unknown datatype.

Suggestion:
appendPQExpBufferStr(buf, ", NULL::pg_catalog.pg_lsn");

2)
+ * Create a replication origin with a specific ID and name, optionally
+ * restoring its remote_lsn.
+ *
+ * The created replication origin is advanced if need_advance is true. It can
+ * be set only in binary upgrade mode.

'optionally restoring its remote_lsn' and 'origin is advanced' seems
like 2 different steps while both are same and are dependent upon
'need_advance'. Suggestion:

Create a replication origin with a specific ID and name, optionally
restoring its remote_lsn when need_advance is true. need_advance can
be set only in binary upgrade mode.

3)
- Commit timestamps and origin data are not preserved during the upgrade.
+ Commit timestamps are not preserved during the upgrade.

We can make it sligtly more clear if you agree. Suggestion:

Commit timestamps (which include the origin of the commit) are not
preserved during the upgrade.

4)

+ old subscriber was up to. Migration of the subscription's replication
+ origin is supported for all cluster versions.

Shall we say:
all supported cluster versions

5)

+ configured to a value greater than or equal to the number of tracked
+ replication origins present in the old cluster (reflected in
+ <link linkend="view-pg-replication-origin-status"><structname>pg_replication_origin_status</structname></link>).
+ The new cluster must contain no replication origins.

To give better continuity in this paragraph, the last sentence can be
changed to:

Furthermore, the new cluster must not contain any pre-existing
replication origins."

thanks
Shveta