Re: Inconsistent error message wording for REINDEX CONCURRENTLY

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.
>
> 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