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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
