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-concurrent for 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 required because 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 relations to
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 the comment 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 transactions open (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