Re: Inconsistent error message wording for REINDEX CONCURRENTLY

On 2019-May-09, Michael Paquier wrote:

> On Wed, May 08, 2019 at 11:28:35PM -0400, Tom Lane wrote:
> > Michael Paquier <michael@paquier.xyz> writes:
> >> 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.
> > 
> > Pushed my stuff, have at it.
> 
> Thanks.  Attached is what I get to after scanning the error messages
> in indexcmds.c and index.c.  Perhaps you have more comments about it?

I do :-)  There are a couple of "is not supported" messages that are
annoyingly similar but different:
git grep --show-function  'reindex.*supported' -- *.c

src/backend/commands/indexcmds.c=ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
src/backend/commands/indexcmds.c:                errmsg("concurrent reindex of system catalogs is not supported")));
src/backend/commands/indexcmds.c:                        errmsg("concurrent reindex is not supported for catalog
relations,skipping all")));
 
src/backend/commands/indexcmds.c=ReindexRelationConcurrently(Oid relationOid, int options)
src/backend/commands/indexcmds.c:                            errmsg("concurrent reindex is not supported for shared
relations")));
src/backend/commands/indexcmds.c:                            errmsg("concurrent reindex is not supported for catalog
relations")));

It seems strange to have some cases say "cannot do foo" and other cases
say "foo is not supported".  However, I think having
ReindexMultipleTables say "cannot reindex a system catalog" would be
slightly wrong (since we're not reindexing one but many) -- so it would
have to be "cannot reindex system catalogs".  And in order to avoid
having two messages that are essentially identical except in number, I
propose to change the others to use the plural too.  So the one you just
committed

> +                /* A system catalog cannot be reindexed concurrently */
> +                if (IsCatalogRelationOid(relationOid))
> +                    ereport(ERROR,
> +                            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                             errmsg("cannot reindex a system catalog concurrently")));

would become "cannot reindex system catalogs concurrently", identical to
the one in ReindexMultipleTables.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services