Re: reindex concurrently and two toast indexes

On Fri, Mar 06, 2020 at 01:36:48PM +0100, Julien Rouhaud wrote:
> 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.

Thanks. The position of the error check in reindex_relation() is
correct, but as it opens a relation for the cache lookup let's invent
a new routine in lsyscache.c to grab pg_index.indisvalid. It is
possible to make use of this routine with all the other checks:
- WARNING for REINDEX TABLE (non-conurrent)
- ERROR for REINDEX INDEX (non-conurrent)
- ERROR for REINDEX INDEX CONCURRENTLY
(There is already a WARNING for REINDEX TABLE CONCURRENTLY.)

I did not find the addition of an error check in ReindexIndex()
consistent with the existing practice to check the state of the
relation reindexed in reindex_index() (for the non-concurrent case)
and ReindexRelationConcurrently() (for the concurrent case). Okay,
this leads to the introduction of two new ERROR messages related to
invalid toast indexes for the concurrent and the non-concurrent cases
when using REINDEX INDEX instead of one, but having two messages leads
to something much more consistent with the rest, and all checks remain
centralized in the same routines.

For the index-level operation, issuing a WARNING is not consistent
with the existing practice to use an ERROR, which is more adapted as
the operation is done on a single index at a time.

For the check in reindex_relation, it is more consistent to check the
namespace of the index instead of the parent relation IMO (the
previous patch used "rel", which refers to the parent table). This
has in practice no consequence though.

It would have been nice to test this stuff. However, this requires an
invalid toast index and we cannot create that except by canceling a
concurrent reindex, leading us back to the upthread discussion about
isolation tests, timeouts and fault injection :/

Any opinions?
--
Michael