Date: Wed, 8 May 2019 16:58:53 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Inconsistent error message wording for REINDEX CONCURRENTLY
Message-ID: <20190508075853.GC1602@paquier.xyz>
References: <23694.1556806002@sss.pgh.pa.us>
 <20190504085543.GE2174@paquier.xyz>
 <4469.1556982011@sss.pgh.pa.us>
 <20190505141645.GA7392@paquier.xyz>
 <16468.1557077559@sss.pgh.pa.us>
 <21697.1557092753@sss.pgh.pa.us>
 <2321.1557263978@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ghzN8eJ9Qlbqn3iT"
Content-Disposition: inline
In-Reply-To: <2321.1557263978@sss.pgh.pa.us>
Precedence: bulk


--ghzN8eJ9Qlbqn3iT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, May 07, 2019 at 05:19:38PM -0400, Tom Lane wrote:
> That's nothing but a hack, and the reason it's necessary is that
> index_create will throw error if IsCatalogRelation is true, which
> it will be for information_schema TOAST tables --- but not for their
> parent tables that are being examined here.

Oh.  Good catch.  That's indeed crazy now that I look closer at that.

> After looking around, it seems to me that the correct definition for
> IsCatalogRelation is just "is the OID less than FirstBootstrapObjectId?".
> Currently we could actually restrict it to "less than
> FirstGenbkiObjectId", because all the catalogs, indexes, and TOAST tables
> have hand-assigned OIDs --- but perhaps someday we'll let genbki.pl
> assign some of those OIDs, so I prefer the weaker constraint.  In any
> case, this gives us a correct separation between objects that are
> traceable to the bootstrap data and those that are created by plain SQL
> later in initdb.
>=20
> With this, the Form_pg_class argument to IsCatalogClass becomes
> vestigial.  I'm tempted to get rid of that function altogether in
> favor of direct calls to IsCatalogRelationOid, but haven't done so
> in the attached.

I think that removing entirely IsCatalogClass() is just better as if
any extension uses this routine, then it could potentially simplify
its code because needing Form_pg_class means usually opening a
Relation, and this can be removed.

With IsCatalogClass() removed, the only dependency with Form_pg_class
comes from IsToastClass() which is not used at all except in
IsSystemClass().  Wouldn't it be better to remove entirely
IsToastClass() and switch IsSystemClass() to use a namespace OID
instead of Form_pg_class?

With your patch, ReindexRelationConcurrently() does not complain for
REINDEX TABLE CONCURRENTLY for a catalog table and would trigger the
error from index_create(), which is at the origin of this thread.  The
check with IsSharedRelation() for REINDEX INDEX CONCURRENTLY is
useless and the error message generated for IsCatalogRelationOid()
still needs to be improved.  Would you prefer to include those changes
in your patch?  Or should I work on top of what you are proposing
(your patch does not include negative tests for toast index and
tables on catalogs either).
--
Michael

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlzSjD0ACgkQnvQgOdby
QH0pog/+KTQ94m2wtb6HYO68w6NTwRnNiN8n7iT6foH6gAbhoBGcyNgGj6AJ4qHF
v57PorZog2COUUfW5yDiJsx2XVEnKzm+aMerNrwBVU8Nf601cvcHI8o3anKTo7ZZ
iiZqrIgKkIbObgZyUwhkcozVCyTCvdBRo6Tx/yWV5Uu421kbeTyoDXnFxY0cUO+i
rg6I0BOJ8l4a96hLkA2d7CTrRXu2NOYrAne42V5MSgDjfghdeUvWu/B28kO/Xwbt
Pgrcjv49/C0df+WRteZC0eVvDgsJhn43myY+b1id2oais5BMH02bFXCWczyrAN1U
3tGVW2do75fAd5POWStk3pIcos5pM4XMATvuNn59XlV80mrUoF00yJTudRbaVvOc
ozxrbVQX1ap2zJk0EwOUKUbGOb0wdQhv2qpT7eFTWJUP/g+UAf2HenttQRzQnpC2
5XysuE7QtO/Hq9Yhknqfpda3FFuqwTElAhh+QwSV6jjbPzr8tXSkk3eQzp7AE4w3
fBAmlAIp1jZiYWayN7VrS3fNGVgCfuazUpDIRXe6gKMnZ+B3WJmj4jCdpeNcA1vS
7/YFYIx25xcCgK5uOklGrvj2dCgGnt1ocM2Lo8C2ohxmlpTC+d5rDtqTIEa7w5D/
FpQsNEmUlMqv+V+APuskOJEgjDc/+30u1VgDnybdA/kyHYJ4leo=
=CYo+
-----END PGP SIGNATURE-----

--ghzN8eJ9Qlbqn3iT--