Re: Proposal : REINDEX SCHEMA

On Mon, Oct 13, 2014 at 5:39 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> > 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"
> >
>
> Thank you for comment and reviewing!
>
> I agree with this.
> I will change syntax to above like that.
>
> >
> > 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.
> >
>
> Yep, it should be same as REINDEX DATABASE.
> IMO, we can put them into one function if they use exactly same logic.
> Thought?
>
> >
> > 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)));
> >
>
> Thanks! I will correct typo.
>
> >
> > 5) Missing of regression tests, please add it to
> > src/test/regress/sql/create_index.sql
> >
> > 6) You need to add psql complete tabs
> >
>
> Next version patch, that I will submit, will have 5), 6) things you
pointed.
>
> > 7) I think we can add "-S / --schema" option do reindexdb in this patch
too.
> > What do you think?
> >
>
> +1 to add "-S / --schema" option
> I was misunderstanding about that.
> I will make the patch which adds this option as separate patch.
>

I registered to the next commitfest [1] and put myself as reviewer.

Regards,

[1] https://commitfest.postgresql.org/action/patch_view?id=1598

--
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