Re: Inconsistent error message wording for REINDEX CONCURRENTLY

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> The message you are referring to in index_create() has been introduced
> as of e093dcdd with the introduction of CREATE INDEX CONCURRENTLY, and
> it can be perfectly hit without REINDEX:
> =# show allow_system_table_mods;
> allow_system_table_mods
> -------------------------
> on
> (1 row)

Oh, yeah, if you do that you can get to it.

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

I concur that we shouldn't need a separate check for relisshared,
since all shared rels should be system catalogs.

I"m not sure I'd move these error-case tests to reindex_catalog.sql ---
bear in mind that later today, that test is either going away entirely
or at least not getting run by default anymore.

regards, tom lane