Re: pg_upgrade and logical replication

On Tue, 12 Sept 2023 at 18:52, Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> 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.

Moved to pg_upgrade_support.c and renamed to binary_upgrade_replorigin_advance

> 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.

Modfied

>
> 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.
>

Modified

> 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.

Modified

>
> 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 ?
I will handle this in the next version.

Thanks for the comments, the v8 patch attached at [1] has the changes
for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm1JzqTreCUrhNu5E1gq7Q8r_u3%2BFrisyT7moOED%3DUdoCg%40mail.gmail.com

Regards,
Vignesh