Re: Inconsistent error message wording for REINDEX CONCURRENTLY

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Tue, May 07, 2019 at 05:19:38PM -0400, Tom Lane wrote:
>> 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.

Yeah, it's clearly easier to use without the extra argument.

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

Not sure. The way it's defined has the advantage of being more
independent of exactly what the implementation of the "is a toast table"
check is. Also, I looked around to see if any callers could really be
simplified if they only had to pass the table OID, and didn't find much;
almost all of them are looking at the pg_class tuple themselves, typically
to check the relkind too. So we'd not make any net savings in syscache
lookups by changing IsSystemClass's API. I'm kind of inclined to leave
it alone.

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

Yes, we still need to do your patch on top of this one (or really
either order would do). I think keeping them separate is good.

BTW, when I was looking at this I got dissatisfied about another
aspect of the wording of the relevant error messages: a lot of them
are like, for example

errmsg("cannot reindex concurrently this type of relation")));

While that matches the command syntax we're using, it's just horrid
English grammar. Better would be

errmsg("cannot reindex this type of relation concurrently")));

Can we change that while we're at it?

regards, tom lane