On Fri, Mar 06, 2020 at 10:38:44AM +0900, Michael Paquier wrote:
> On Thu, Mar 05, 2020 at 05:57:07PM +0100, Julien Rouhaud wrote:
> > I agree that the approach wasn't quite robust. I'll try to look at adding a
> > new command for isolationtester, but that's probably not something we want to
> > put in pg13?
>
> Yes, that's too late.
>
> > Note that while looking at it, I noticed another bug in RIC:
> >
> > [...]
> >
> > # reindex table concurrently t1;
> > WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
> > LOCATION: ReindexRelationConcurrently, indexcmds.c:2821
> > WARNING: XX002: cannot reindex invalid index "pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
> > LOCATION: ReindexRelationConcurrently, indexcmds.c:2867
> > REINDEX
> > # reindex index concurrently t1_val_idx_ccold;
> > REINDEX
> >
> > That case is also fixed in this patch.
>
> This choice is intentional. The idea about bypassing invalid indexes
> for table-level REINDEX is that this would lead to a bloat in the
> number of relations to handling if multiple runs are failing, leading
> to more and more invalid indexes to handle each time. Allowing a
> single invalid non-toast index to be reindexed with CONCURRENTLY can
> be helpful in some cases, like for example a CIC for a unique index
> that failed and was invalid, where the relation already defined can be
> reused.
Ah I see, thanks for the clarification. I guess there's room for improvement
in the comments about that, since the ERRCODE_FEATURE_NOT_SUPPORTED usage is
quite misleading there.
v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
non-TOAST index anymore.