Re: Inconsistent error message wording for REINDEX CONCURRENTLY - Mailing list pgsql-hackers

I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> What do you think about something like the attached then?  HEAD does
>> not check after system indexes with REINDEX INDEX CONCURRENTLY, and I
>> have moved all the catalog-related tests to reindex_catalog.sql.

> OK as far as the wording goes, but now that I look at the specific tests
> that are being applied, they seem rather loony, as well as inconsistent
> between the two cases.  IsSystemRelation *sounds* like the right thing,
> but it's not, because it forbids user-relation toast tables which seems
> like a restriction we need not make.  I think IsCatalogRelation is the
> test we actually want there.  In the other place, checking
> IsSystemNamespace isn't even approximately the correct way to proceed,
> since it fails to reject reindexing system catalogs' toast tables.
> We should be doing the equivalent of IsCatalogRelation there too.
> (It's a bit of a pain that catalog.c doesn't offer a function that
> makes that determination from just an OID.  Should we add one?
> There might be other callers for it.)

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.

I was thinking about also proposing that we rename IsSystemRelation
to IsCatalogOrToastRelation (likewise for IsSystemClass), which would
be clearer as to its semantics.  However, after looking through the
code, it seems that 90% of the callers are using those functions to
decide whether to apply !allow_system_table_mods restrictions, and
indeed it's likely that some of the other 10% are wrong and should be
testing IsCatalogRelation/Class instead.  So unless we want to rename
that GUC, I think the existing names of these functions are fine but
we should adjust their comments to explain that this is the primary
if not sole use-case.  Another idea is to make IsSystemRelation/Class
be macros for IsCatalogOrToastRelation/Class, with the intention that
we use the former names specifically for allow_system_table_mods tests
and the latter names for anything else that happens to really want
those semantics.

There's some other cleanup I want to do in catalog.c --- many of the
comments are desperately in need of copy-editing, to start with ---
but I don't think any of the rest of it would be controversial.

            regards, tom lane