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.gc30...@paquier.xyz -- Michael
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 412e1ba84f..05570eb3f3 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3524,7 +3524,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
if (!indexRelation->rd_index->indisvalid)
ereport(WARNING,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
get_namespace_name(get_rel_namespace(cellOid)),
get_rel_name(cellOid))));
@@ -3576,7 +3576,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
if (!indexRelation->rd_index->indisvalid)
ereport(WARNING,
- (errcode(ERRCODE_INDEX_CORRUPTED),
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
get_namespace_name(get_rel_namespace(cellOid)),
get_rel_name(cellOid))));
signature.asc
Description: PGP signature
