Обсуждение: reindexing an invalid index should not use ERRCODE_INDEX_CORRUPTED

Hi,

We currently provide no way to learn about a postgres instance having
corruption than searching the logs for corruption events than matching by
sqlstate, for ERRCODE_DATA_CORRUPTED and ERRCODE_INDEX_CORRUPTED.

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?

Greetings,

Andres Freund

On Sat, Nov 18, 2023 at 03:09:58PM -0800, Andres Freund wrote:
> We currently provide no way to learn about a postgres instance having
> corruption than searching the logs for corruption events than matching by
> sqlstate, for ERRCODE_DATA_CORRUPTED and ERRCODE_INDEX_CORRUPTED.
> 
> 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 "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.  Since an INVALID
index often duplicates some valid index, I could see an argument that
reindexing INVALID indexes as part of a table-level REINDEX is wanted less
often than not.  But that argument would be just as pertinent to REINDEX TABLE
(w/o CONCURRENTLY), which doesn't impose this restriction.  Hmmm.

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?

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

> Since an INVALID
> index often duplicates some valid index, I could see an argument that
> reindexing INVALID indexes as part of a table-level REINDEX is wanted less
> often than not.

The argument behind this restriction is that repeated interruptions of
a table-level REINDEX CONCURRENTLY would bloat the entire relation in
index entries if invalid entries are rebuilt.  This was discussed back
on the original thread back in 2019, around here:
https://www.postgresql.org/message-id/20190411132704.GC30766@paquier.xyz
--
Michael

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.

On Wed, Dec 06, 2023 at 04:33:33PM -0800, Noah Misch wrote:
> On Wed, Dec 06, 2023 at 03:17:12PM +0900, Michael Paquier wrote:
>>> 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,

In line with vacuum.c, that sounds like a good idea at the end.

> errhint("Use DROP INDEX or REINDEX INDEX.") would fit.

I'm OK with this suggestion as well.
--
Michael

Hi,

This looks good to me!

Greetings,

Andres Freund

On Wed, Dec 06, 2023 at 05:40:44PM -0800, Andres Freund wrote:
> This looks good to me!

Cool.  I've applied this one, then.
--
Michael