Re: Inconsistent error message wording for REINDEX CONCURRENTLY

On Wed, May 08, 2019 at 08:31:54AM -0400, Tom Lane wrote:
> Michael Paquier <michael(at)paquier(dot)xyz> writes:
>> 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.

Hmm. Okay. It would have been nice to remove completely the
dependency to Form_pg_class from this set of APIs, but I can see your
point.

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

Okay, glad to hear. That's what I wanted to do.

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

No problem to do that. I'll brush up all that once you commit the
first piece you have come up with, and reuse the new API of catalog.c
you are introducing based on the table OID.
--
Michael