On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello <fabriziome...@gmail.com> wrote: > > > On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello > <fabriziome...@gmail.com> wrote: >> >> >> 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. >> > > Patch attached. Now the regress run without errors. >
Thank you for reviewing and revising! I also did successfully. It looks good to me :) Regards, ------- Sawada Masahiko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
