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.