Re: pg_upgrade and logical replication

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.

Like the publisher-side upgrade patch [1], I think we should allow
upgrading subscriptions by default instead with some flag like
--preserve-subscription-state. If required, we can introduce --exclude
option for upgrade. Having it just for pg_dump sounds reasonable to
me.

[1] - https://www.postgresql.org/message-id/TYAPR01MB58664C81887B3AF2EB6B16E3F5939%40TYAPR01MB5866.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.