On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.m...@gmail.com> wrote: > > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello > <fabriziome...@gmail.com> wrote: > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <sawada.m...@gmail.com > > > wrote: > >> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmh...@gmail.com> > >> wrote: > >> > On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <sfr...@snowman.net> > >> > wrote: > >> >> * Alvaro Herrera (alvhe...@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. > Also, I noticed I had forgotten to add the patch regarding document of > reindexdb. > Yeah... I forgot it too... :-) I'm not a native speaker but IMHO the docs is fine. 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
