RE: pg_upgrade and logical replication

On Monday, September 11, 2023 6:32 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
>
> The attached v7 patch has the changes for the same.

Thanks for updating the patch, here are few comments:

1.

+/*
+ * binary_upgrade_sub_replication_origin_advance
+ *
+ * Update the remote_lsn for the subscriber's replication origin.
+ */
+Datum
+binary_upgrade_sub_replication_origin_advance(PG_FUNCTION_ARGS)
+{

Is there any usage apart from pg_upgrade for this function, if not, I think
we'd better move this function to pg_upgrade_support.c. If yes, I think maybe
better to rename it to a general one.

2.

+ * Verify that all subscriptions have a valid remote_lsn and don't contain
+ * any table in srsubstate different than ready ('r').
+ */
+static void
+check_for_subscription_state(ClusterInfo *cluster)

I think we'd better follow the same style of
check_for_isn_and_int8_passing_mismatch() to record the invalid things in a
file.

3.

+ if (fout->dopt->binary_upgrade && fout->remoteVersion >= 100000)
+ {
+ appendPQExpBuffer(query,
+ "SELECT binary_upgrade_create_sub_rel_state('%s', %u, '%c'",
+ subrinfo->dobj.name,

I think we'd better consider using appendStringLiteral or related function for
the dobj.name here to make sure the string convertion is safe.

4.

The following commit message may need update:
"binary_upgrade_create_sub_rel_state SQL function, and also provides an
additional LSN parameter for CREATE SUBSCRIPTION to restore the underlying
replication origin remote LSN. "

I think we have changed to another approach which doesn't provide new parameter
in DDL.

5.
+ /* Fetch the existing tuple. */
+ tup = SearchSysCacheCopy2(SUBSCRIPTIONNAME, MyDatabaseId,
+ CStringGetDatum(subname));

Since we don't modify the tuple here, SearchSysCache2 seems enough.

6.
+ "LEFT JOIN pg_catalog.pg_database d"
+ " ON d.oid = s.subdbid "
+ "WHERE coalesce(remote_lsn, '0/0') = '0/0'");

For the subscriptions that were just created and finished the table sync but
haven't applied any changes, their remote_lsn will also be 0/0. Do we
need to report ERROR in this case ?

Best Regards,
Hou zj