Re: pg_upgrade and logical replication

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.

> 2. state V relstate
> 
> I still feel code readbility suffers a bit by calling some fields/vars
> a generic 'state' instead of the more descriptive 'relstate'. Maybe
> it's just me.
> 
> Previously commented same (see [1]#3, #4, #5)

Agreed to be more careful with the naming here.
--
Michael