Re: BUG #18630: Incorrect memory access inside ReindexIsProcessingIndex() on VACUUM

Michael Paquier <michael(at)paquier(dot)xyz> 于2024年9月26日周四 15:02写道：

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

--
Thanks,
Tender Wang
https://www.openpie.com/