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 :(