Re: Inconsistent error message wording for REINDEX CONCURRENTLY

I wrote:
> After looking around a bit, I propose that we invent
> "IsCatalogRelationOid(Oid reloid)" (not wedded to that name), which
> is a wrapper around IsCatalogClass() that does the needful syscache
> lookup for you. Aside from this use-case, it could be used in
> sepgsql/dml.c, which I see is also using
> IsSystemNamespace(get_rel_namespace(oid)) for the wrong thing.

> I'm also thinking that it'd be a good idea to rename IsSystemNamespace
> to IsCatalogNamespace. The existing naming is totally confusing given
> that it doesn't square with the distinction between IsSystemRelation
> and IsCatalogRelation (ie that the former includes user toast tables).
> There are only five external callers of it, and per this discussion
> at least two of them are wrong anyway.

After studying the callers of these catalog.c functions for awhile,
I realized that IsCatalogRelation/Class are really fundamentally wrong,
and have been so for a long time. The reason is that while they will
return FALSE for tables in the information_schema, they will return
TRUE for toast tables attached to the information_schema tables.
(They're toast tables, and they have OIDs below FirstNormalObjectId,
so there you have it.) This is wrong on its face: if those tables don't
need to be protected as catalogs, why should their TOAST appendages
need it? Moreover, if you drop and recreate information_schema, you'll
start getting different behavior for them, which is even sillier.

I was driven to this realization by the following very confused (and
confusing) bit in ReindexMultipleTables:

/*
* Skip system tables that index_create() would reject to index
* concurrently. XXX We need the additional check for
* FirstNormalObjectId to skip information_schema tables, because
* IsCatalogClass() here does not cover information_schema, but the
* check in index_create() will error on the TOAST tables of
* information_schema tables.
*/
if (concurrent &&
(IsCatalogClass(relid, classtuple) || relid < FirstNormalObjectId))
{

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.

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.

Comments?

regards, tom lane