Hi,
Looking at this version of the patch now:
1) comment for "Phase 4 of REINDEX CONCURRENTLY" ends with an incomplete
sentence.
2) I don't think the drop algorithm used now is correct. Your
index_concurrent_set_dead() sets both indisvalid = false and indislive =
false at the same time. It does so after doing a WaitForVirtualLocks() -
but that's not sufficient. Between waiting and setting indisvalid =
false another transaction could start which then would start using that
index. Which will not get updated anymore by other concurrent backends
because of inislive = false.
You really need to follow index_drop's lead here and first unset
indisvalid then wait till nobody can use the index for querying anymore
and only then unset indislive.
3) I am not sure if the swap algorithm used now actually is correct
either. We have mvcc snapshots now, right, but we're still potentially
taking separate snapshot for individual relcache lookups. What's
stopping us from temporarily ending up with two relcache entries with
the same relfilenode?
Previously you swapped names - I think that might end up being easier,
because having names temporarily confused isn't as bad as two indexes
manipulating the same file.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
