Re: reindexing an invalid index should not use ERRCODE_INDEX_CORRUPTED

On Wed, Dec 06, 2023 at 03:17:12PM +0900, Michael Paquier wrote:
> On Sat, Nov 18, 2023 at 04:32:36PM -0800, Noah Misch wrote:
> > On Sat, Nov 18, 2023 at 03:09:58PM -0800, Andres Freund wrote:
> >> Unfortunately, there is a case of such an sqlstate that's not at all indicating
> >> corruption, namely REINDEX CONCURRENTLY when the index is invalid:
> >> 
> >>                         if (!indexRelation->rd_index->indisvalid)
> >>                             ereport(WARNING,
> >>                                     (errcode(ERRCODE_INDEX_CORRUPTED),
> >>                                      errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
> >>                                             get_namespace_name(get_rel_namespace(cellOid)),
> >>                                             get_rel_name(cellOid))));
> >> 
> >> The only thing required to get to this is an interrupted CREATE INDEX
> >> CONCURRENTLY, which I don't think can be fairly characterized as "corruption".
> >> 
> >> ISTM something like ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be more
> >> appropriate?
> > 
> > +1, that's a clear improvement.
> 
> The same thing can be said a couple of lines above where the code uses
> ERRCODE_FEATURE_NOT_SUPPORTED but your suggestion of
> ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE would be better.
> 
> Would the attached be OK for you?

Okay.

> > The "cannot" part of the message is also inaccurate, and it's not clear to me
> > why we have this specific restriction at all.  REINDEX INDEX CONCURRENTLY
> > accepts such indexes, so I doubt it's an implementation gap.
> 
> If you would reword that, what would you change?

I'd do "skipping reindex of invalid index \"%s.%s\"".  If one wanted more,
errhint("Use DROP INDEX or REINDEX INDEX.") would fit.