Re: pg_upgrade and logical replication

On Mon, 4 Sept 2023 at 13:26, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Sep 4, 2023 at 11:51 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Jul 19, 2023 at 12:47 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > >
> > > On Wed, May 10, 2023 at 05:59:24PM +1000, Peter Smith wrote:
> > > > 1. ALTER SUBSCRIPTION name ADD TABLE (relid = XYZ, state = 'x' [, lsn = 'X/Y'])
> > > >
> > > > I was a bit confused by this relation 'state' mentioned in multiple
> > > > places. IIUC the pg_upgrade logic is going to reject anything with a
> > > > non-READY (not 'r') state anyhow, so what is the point of having all
> > > > the extra grammar/parse_subscription_options etc to handle setting the
> > > > state when only possible value must be 'r'?
> > >
> > > We are just talking about the handling of an extra DefElem in an
> > > extensible grammar pattern, so adding the state field does not
> > > represent much maintenance work. I'm OK with the addition of this
> > > field in the data set dumped, FWIW, on the ground that it can be
> > > useful for debugging purposes when looking at --binary-upgrade dumps,
> > > and because we aim at copying catalog contents from one cluster to
> > > another.
> > >
> > > Anyway, I am not convinced that we have any need for a parse-able
> > > grammar at all, because anything that's presented on this thread is
> > > aimed at being used only for the internal purpose of an upgrade in a
> > > --binary-upgrade dump with a direct catalog copy in mind, and having a
> > > grammar would encourage abuses of it outside of this context. I think
> > > that we should aim for simpler than what's proposed by the patch,
> > > actually, with either a single SQL function à-la-binary_upgrade() that
> > > adds the contents of a relation. Or we can be crazier and just create
> > > INSERT queries for pg_subscription_rel to provide an exact copy of the
> > > catalog contents. A SQL function would be more consistent with other
> > > objects types that use similar tricks, see
> > > binary_upgrade_create_empty_extension() that does something similar
> > > for some pg_extension records. So, this function would require in
> > > input 4 arguments:
> > > - The subscription name or OID.
> > > - The relation OID.
> > > - Its LSN.
> > > - Its sync state.
> > >
> >
> > +1 for doing it via function (something like
> > binary_upgrade_create_sub_rel_state). We already have the internal
> > function AddSubscriptionRelState() that can do the core work.
> >

Modified

> One more related point:
> @@ -4814,9 +4923,31 @@ dumpSubscription(Archive *fout, const
> SubscriptionInfo *subinfo)
> if (strcmp(subinfo->subpasswordrequired, "t") != 0)
> appendPQExpBuffer(query, ", password_required = false");
>
> + if (dopt->binary_upgrade && dopt->preserve_subscriptions &&
> + subinfo->suboriginremotelsn)
> + {
> + appendPQExpBuffer(query, ", lsn = '%s'", subinfo->suboriginremotelsn);
> + }
>
> Even during Create Subscription, we can use an existing function
> (pg_replication_origin_advance()) or a set of functions to advance the
> origin instead of introducing a new option.

Added a function binary_upgrade_sub_replication_origin_advance which
will: a) check if the subscription exists, b) get the replication name
for subscription and c) advance the replication origin.

These are handled as part of v7 posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com

Regards,
Vignesh