Re: PATCH: Issue with set_indexsafe_procflags in ReindexRelationConcurrently

On Fri, Sep 06, 2024 at 01:27:12PM +0200, Michail Nikolaev wrote:
> While working on [1], I have found a small issue with correctness
> of set_indexsafe_procflags usage in ReindexRelationConcurrently introduced
> in [2].
>
> > idx->safe = (indexRel->rd_indexprs == NIL && indexRel->rd_indpred == NIL);
>
> It is always true because there are no RelationGetIndexExpressions
> and RelationGetIndexPredicate before that check.
>
> Two patches with reproducer + fix are attached.
>
> The issue is simple, but I'll register this in commitfest just in case.

Ugh. It means that we've always considered as "safe" concurrent
rebuilds of indexes that have expressions or predicates, but they're
not safe at all. Other concurrent jobs should wait for them.

Adding these two calls as you are suggesting is probably a good idea
anyway to force a correct setup of the flag. Will see about that.

I don't understand why an isolation test is required here if we were
to add a validity test, because you can cause a failure in the REINDEX
with a set of SQLs in a single session. I'm OK to add a test, perhaps
with a NOTICE set when the safe flag is true. Anyway, what you have
is more than enough to prove your point. Thanks for that.
--
Michael