On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <sfrost@snowman.net> wrote: > > * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > > Sawada Masahiko wrote: > > > Attached WIP patch adds new syntax REINEX SCHEMA which does reindexing > > > all table of specified schema. > > > There are syntax dose reindexing specified index, per table and per database, > > > but we can not do reindexing per schema for now. > > > > It seems doubtful that there really is much use for this feature, but if > > there is, I think a better syntax precedent is the new ALTER TABLE ALL > > IN TABLESPACE thingy, rather than your proposed REINDEX SCHEMA. > > Something like REINDEX TABLE ALL IN SCHEMA perhaps. > > Yeah, I tend to agree that we should be looking at the 'ALL IN > TABLESPACE' and 'ALL IN SCHEMA' type of commands to keep things > consistent. This might be an alternative for the vacuum / analyze / > reindex database commands also.. >
Some review:
1) +1 to "REINDEX ALL IN SCHEMA name"
2) IMHO the logic should be exactly the same as REINDEX DATABASE, including the transaction control. Imagine a schema with a lot of tables, you can lead to a deadlock using just one transaction block.
3) The patch was applied to master and compile without warnings
4) Typo (... does not have any table)
+ if (!reindex_schema(heapOid)) + ereport(NOTICE, + (errmsg("schema\"%s\" does not hava any table", + schema->relname)));
5) Missing of regression tests, please add it to src/test/regress/sql/create_index.sql
6) You need to add psql complete tabs
7) I think we can add "-S / --schema" option do reindexdb in this patch too. What do you think?
