On Wed, Jul 19, 2023 at 12:47 PM Michael Paquier <mich...@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.
