Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 19 Jul 2023 16:17:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>,
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: pg_upgrade and logical replication
Message-ID: <ZLeODIhuoClIQTPR@paquier.xyz>
References: 
 <CAA4eK1+4ORS7C+j3NS9yfoKdjfpdat0XrhUy1opBvw8WN58g4Q@mail.gmail.com>
 <CAOBaU_YavOL0AhS0uU07Ea_0ZyR7youtJAvSHDnhmADGDw+VwA@mail.gmail.com>
 <CAA4eK1+pWXqDSO+QpGtLFqcCP5+qCXkvLN-YviRqdXV+d7Fdow@mail.gmail.com>
 <20230309083456.xpjcsuycyn7d2yvr@jrouhaud>
 <20230327084955.m624nwfaf7mbzo6j@jrouhaud>
 <TYAPR01MB58667340763002108CEB5EB8F5919@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20230407022802.jqmdhgjhrnj5lpcl@jrouhaud>
 <TYAPR01MB586682CF17623F6E71B9D3ADF59D9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20230424072224.qd3awkfrjcanw2pa@jrouhaud>
 <CAHut+PvYt0yVJ+eAg0AdTHyTmgoZVY9znk3uxe98jOytmeCwLA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sj06OEQZLTX4lC1e"
Content-Disposition: inline
In-Reply-To: 
 <CAHut+PvYt0yVJ+eAg0AdTHyTmgoZVY9znk3uxe98jOytmeCwLA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZLeODIhuoClIQTPR%40paquier.xyz>
Precedence: bulk


--sj06OEQZLTX4lC1e
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, May 10, 2023 at 05:59:24PM +1000, Peter Smith wrote:
> 1. ALTER SUBSCRIPTION name ADD TABLE (relid =3D XYZ, state =3D 'x' [, lsn=
 =3D 'X/Y'])
>=20
> 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 =E0-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
>=20
> 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.
>=20
> Previously commented same (see [1]#3, #4, #5)

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

--sj06OEQZLTX4lC1e
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmS3jgwACgkQnvQgOdby
QH3pIQ/+KuSy8EuR62Nuvk1d/3iL9tYmDqtOVjNGvfy7HfSi3aEYtrwYnfboXX2y
gFAt4GXffVMpR8+qcbUBfJhsEdnUKBrmploFq6Hb58b7dxudLMvoy93bvDYsh05Z
TkVYQDkDqgd4cI2/pecDebtNubOIL/C5beu5kQB4wEhfDFq3yDuKIZeaMBSI1l0c
VuXejNYhX6Lv4lTnquM6NWHjqawFFgBv8OOx1MlRp/7/vDEeu7XWLXjOUcG3QUnc
WomE1VVVhspeyYMVx4DjiB7gXV23vQDu7MkmhbQPrJqrI4i+KgBvGszdlE3bf0FM
o8gMJfAL+g8D7JFWFHlP/hx4qBHSU75QNL58VUZoXp3TdD+fSElnBJjATn6U+gkI
lRARFFlTiS1B121PKhge/KrQPaaq4IzLdC8kABKbH9HYY7ByI9BHZbqRZRd2+CKB
Ek/Ks8GNdnShhqrKtIz6+3TajGtaKzwxjLjZ9+DjCnDY7IvDVSvd3OgJyKSAjLWR
srqNL5xiJLSAfyPRZ9KBm3G1DO3l9qpjwgSE+tXp3dQS4vQvN1Ik227dvroOLolj
CQEJLLQYocYxeGyKYw3UYLWB3F9H8QyN8m83Snf60LhJN6wyyICg0g0SMYAeGP2z
WJv2f0ZbSZW9FFgTH+FPG+8y7SsUXGT/dKvBrqmUJSodUQpJLKs=
=Caqm
-----END PGP SIGNATURE-----

--sj06OEQZLTX4lC1e--