On Wed, Sep 25, 2024 at 06:19:20PM +0800, Tender Wang wrote: > Adding if (flags & REINDEX_REL_SUPPRESS_INDEX_USE) check before calling > RemoveReindexPending() may look better. > Any thoughts?
The short answer to that is yes, see below.
Alexander's trick presented upthread is funky. I was first confused of what was the point until I noticed that this is just a way to make REINDEX CONCURRENTLY fail and produce an invalid toast index. The first pg_sleep causes REINDEX to fail because we are waiting for the first transaction to complete. So this provides a controlled way to get an index that should never be part of the list reported to SetReindexPending() to begin with. The VACUUM (PROCESS_MAIN FALSE, FULL) is then disturbed.
Anyway, PROCESS_MAIN is not related to the failure, as it is just a flavor grammar introduced for the sake of making toast-only rebuilds easier for administrators. The same failure reproduces if switching the test case of upthread to do the REINDEX on the toast table, down to 12 which is as far as I have tested because that's what we support around here.
Regarding the fix, we should not disturb the list of indexes in a relation returned by relcache.c, and invalid indexes are part of it. So I'd agree with your point to just remove the index from the pending list because we have to skip invalid toast indexes in the reindex_relation() path as reindex_index() has to generate a hard failure because we can never ever have two valid toast indexes, and that's a guarantee we need to be very careful about.
Now, about REINDEX_REL_SUPPRESS_INDEX_USE. Well, that's only used for the VACUUM FULL/CLUSTER path which is where the indexes are marked as being processed, so we have to reomve the invalid toast indexes from the list only if this flag is used. Hence the answer is yes. I'd rather keep the informative warning. That's useful for monitoring purposes.
Agree. The attached patch LGTM.
Something worth noting while I have looked at this code.. Non-toast invalid indexes are rebuilt but still marked as !indisvalid, actually. That's a waste and we could just skip them for a CLUSTER/VACUUM FULL. This cannot be true for a REINDEX TABLE, invalid indexes are switched to valid in this case.
