Re: pg_upgrade and logical replication

On Thu, 7 Dec 2023 at 07:20, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Dec 4, 2023 at 8:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Dec 1, 2023 at 11:24 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > The attached v22 version patch has the changes for the same.
> > >
> >
> > I have made minor changes in the comments and code at various places.
> > See and let me know if you are not happy with the changes. I think
> > unless there are more suggestions or comments, we can proceed with
> > committing it.
> >
>
> It seems the patch is already close to ready-to-commit state but I've
> had a look at the v23 patch with fresh eyes. It looks mostly good to
> me and there are some minor comments:
>
> ---
> + tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
> + if (!HeapTupleIsValid(tup))
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("relation %u does not exist", relid));
> + ReleaseSysCache(tup);
>
> Given what we want to do here is just an existence check, isn't it
> clearer if we use SearchSysCacheExists1() instead?

Modified

> ---
> + query = createPQExpBuffer();
> + appendPQExpBuffer(query, "SELECT srsubid, srrelid,
> srsubstate, srsublsn"
> + " FROM
> pg_catalog.pg_subscription_rel"
> + " ORDER BY srsubid");
> + res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
> +
>
> Probably we don't need to use PQExpBuffer here since the query to
> execute is a static string.

Modified

> ---
> +# The subscription's running status should be preserved. Old subscription
> +# regress_sub1 should be enabled and old subscription regress_sub2 should be
> +# disabled.
> +$result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FROM pg_subscription ORDER BY subname");
> +is( $result, qq(t
> +f),
> + "check that the subscription's running status are preserved");
> +
>
> How about showing the subname along with the subenabled so that we can
> check if each subscription is in an expected state in case where
> something error happens?

Modified

> ---
> +# Subscription relations should be preserved
> +$result =
> + $new_sub->safe_psql('postgres',
> + "SELECT count(*) FROM pg_subscription_rel WHERE srsubid = $sub_oid");
> +is($result, qq(2),
> + "there should be 2 rows in pg_subscription_rel(representing
> tab_upgraded1 and tab_upgraded2)"
> +);
>
> Is there any reason why we check only the number of rows in
> pg_subscription_rel? I guess it might be a good idea to check if table
> OIDs there are also preserved.

Modified

> ---
> +# Enable the subscription
> +$new_sub->safe_psql('postgres', "ALTER SUBSCRIPTION regress_sub2 ENABLE");
> +$publisher->wait_for_catchup('regress_sub2');
> +
>
> IIUC after making the subscription regress_sub2 enabled, we will start
> the initial table sync for the table tab_upgraded2. If so, shouldn't
> we use wait_for_subscription_sync() instead?

Modified

> ---
> +# Create another subscription and drop the subscription's replication origin
> +$old_sub->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_sub4 CONNECTION '$connstr'
> PUBLICATION regress_pub3 WITH (enabled=false)"
>
> It's better to put spaces before and after '='.

Modified

> ---
> +my $subid = $old_sub->safe_psql('postgres',
> + "SELECT oid FROM pg_subscription WHERE subname = 'regress_sub4'");
>
> I think we can reuse $sub_oid.

Modified

Thanks for the comments, the v24 version patch attached at [1] has the
changes for the same.
[1] - https://www.postgresql.org/message-id/CALDaNm27%2BB6hiCS3g3nUDpfwmTaj6YopSY5ovo2%3D__iOSpkPbA%40mail.gmail.com

Regards,
Vignesh