Re: Support for REINDEX CONCURRENTLY

On 2012-12-10 15:51:40 +0100, Andres Freund wrote:
> On 2012-12-10 15:03:59 +0900, Michael Paquier wrote:
> > I have updated the patch (v4) to take care of updating reltoastidxid for
> > toast parent relations at the swap step by using index_update_stats. In
> > prior versions of the patch this was done when concurrent index was built,
> > leading to toast relations using invalid indexes if there was a failure
> > before the swap phase. The update of reltoastidxids of toast relation is
> > done with RowExclusiveLock.
> > I also added a couple of tests in src/test/isolation. Btw, as for the time
> > being the swap step uses AccessExclusiveLock to switch old and new
> > relnames, it does not have any meaning to run them...
>
> Btw, as an example of the problems caused by renaming:
> Looking at the patch for a bit now.

Some review comments:

* Some of the added !is_reindex in index_create don't seem safe to me. Why do we now support reindexing exlusion
constraints?

* REINDEX DATABASE .. CONCURRENTLY doesn't work, a variant that does the concurrent reindexing for user-tables and
non-concurrentfor system tables would be very useful. E.g. for the upgrade from 9.1.5->9.1.6...
 

* ISTM index_concurrent_swap should get exlusive locks on the relation *before* printing their names. This shouldn't be
requiredbecause we have a lock prohibiting schema changes on the parent table, but it feels safer.
 

* temporary index names during swapping should also be named via ChooseIndexName

* why does create_toast_table pass an unconditional 'is_reindex' to index_create?

* would be nice (but thats probably a step #2 thing) to do the individual steps of concurrent reindex over multiple
relationsto avoid too much overall waiting for other transactions.
 

* ReindexConcurrentIndexes:
 * says " Such indexes are simply bypassed if caller has not specified   anything." but ERROR's. Imo ERROR is fine, but
thecomment should be   adjusted...
 
 * should perhaps be names ReindexIndexesConcurrently?
 * Imo the PHASE 1 comment should be after gathering/validitating the   chosen indexes
 * It seems better to me to do use individual transactions + snapshots   for each index, no need to keep very long
transactionsopen (PHASE   2/3)
 
 * s/same whing/same thing/
 * Shouldn't a CacheInvalidateRelcacheByRelid be done after PHASE 2 and   5 as well?
 * PHASE 6 should acquire exlusive locks on the indexes

* can some of index_concurrent_* infrastructure be reused for DROP INDEX CONCURRENTLY?

* in CREATE/DROP INDEX CONCURRENTLY 'CONCURRENTLY comes before the object name, should we keep that conventioN?


Thats all I have for now.


Very nice work! Imo the code looks cleaner after your patch...


Greetings,

Andres Freund

--Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services