RE: pg_upgrade and logical replication

On Monday, September 11, 2023 6:32 PM vignesh C <vignesh21@gmail.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