On Mon, Mar 09, 2020 at 02:52:31PM +0900, Michael Paquier wrote:
> On Fri, Mar 06, 2020 at 01:36:48PM +0100, Julien Rouhaud wrote:
> >
> > 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.
I wanted to go this way at first but hesitated and finally chose to add less
checks, so I'm fine with this approach, and patch looks good to me.
> 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.
Agreed.
> 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.
Oops yes.
> 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 :/
Yes, unfortunately I don't see an acceptable way to add tests for that without
some kind of fault injection, so this will have to wait :(