Re: Proposal : REINDEX SCHEMA

On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
> > <fabriziomello@gmail.com> wrote:
> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.mshk@gmail.com>
> > > wrote:
> > >>
> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas@gmail.com>
> > >> wrote:
> > >> > On Sun, Oct 12, 2014 at 1: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..
> > >> >
> > >> > Urgh.  I don't have a problem with that syntax in general, but it
> > >> > clashes pretty awfully with what we're already doing for REINDEX
> > >> > otherwise.
> > >> >
> > >>
> > >> Attached patches are latest version patch.
> > >
> > > Ok.
> > >
> > >
> > >> I changed syntax to REINDEX ALL IN SCHEMA, but I felt a sense of
> > >> discomfort a little
> > >> as Robert mentioned.
> > >>
> > >
> > > I understood, but the real problem will in a near future when the features
> > > will be pushed... :-)
> > >
> > > They are separated features, but maybe we can join this features to a one
> > > future commit... it's just an idea.
> > >
> > >
> > >> Anyway, you can apply these patches in numerical order,
> > >> can use REINDEX ALL IN SCHEMA feature and  "-S/--schema" option in
> > >> reindexdb.
> > >>
> > >> 000_reindex_all_in_schema_v2.patch : It contains REINDEX ALL IN SCHEMA
> > >> feature
> > >
> > > 1) Compile without warnings
> > >
> > >
> > > 2) IMHO you can add more test cases to better code coverage:
> > >
> > > * reindex a schema that doesn't exists
> > > * try to run "reindex all in schema" inside a transaction block
> > >
> > >
> > > 3) Isn't enough just?
> > >
> > > bool do_database = (kind == OBJECT_DATABASE);
> > >
> > > ... instead of...
> > >
> > > +   bool do_database = (kind == OBJECT_DATABASE) ? true : false;
> > >
> > >
> > > 4) IMHO you can add other Assert to check valid relkinds, like:
> > >
> > > Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
> > >
> > >
> > > 5) I think is more legible:
> > >
> > > /* Get OID of object for result */
> > > if (do_database)
> > >     objectOid = MyDatabaseId
> > > else
> > >     objectOid = get_namespace_oid(objectName, false);
> > >
> > > ... insead of ...
> > >
> > > +   /* Get OID of object for result */
> > > +   objectOid = (do_database) ? MyDatabaseId : get_namespace_oid(objectName,
> > > false);
> > >
> > >
> > >
> > >> 001_Add_schema_option_to_reindexdb_v1.patch : It contains reindexdb
> > >> "-S/--schema" supporting
> > >>
> > >
> > > The code itself is good for me, but  IMHO you can add test cases to
> > > src/bin/scripts/t/090_reindexdb.pl
> > >
> >
> > Thank you for reviewing.
>
> You're welcome!
>
>
> > I agree 2) - 5).
>
> :-)
>
>
> > Attached patch is latest version patch I modified above.
>
> All is fine to me now... all work as expected and no compiler warnings.
>
> There are just a little fix to do in src/bin/scripts/t/090_reindexdb.pl
>
> -use Test::More tests => 7;
> +use Test::More tests => 8;
>
> Because you added a new testcase to suittest, so you need to increase the test count at beginning of the file.
>

Patch attached. Now the regress run without errors.

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