Re: pg_upgrade and logical replication

On Mon, Sep 4, 2023 at 11:51 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Jul 19, 2023 at 12:47 PM Michael Paquier <michael@paquier.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.
>

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.

--
With Regards,
Amit Kapila.