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