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
signature.asc
Description: PGP signature
