Re: Proposal : REINDEX SCHEMA - Mailing list pgsql-hackers

On Mon, Oct 13, 2014 at 5:39 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > 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"
> >
>
> 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.
>

[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