Re: Proposal : REINDEX SCHEMA

On Sun, Oct 12, 2014 at 2:27 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)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?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello