Re: Proposal : REINDEX SCHEMA

On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
>
> On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>>
>> On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>> wrote:
>> >
>> > On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
>> > <fabriziomello(at)gmail(dot)com> wrote:
>> > > On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko
>> > > <sawada(dot)mshk(at)gmail(dot)com>
>> > > wrote:
>> > >>
>> > >> On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
>> > >> wrote:
>> > >> > On Sun, Oct 12, 2014 at 1: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..
>> > >> >
>> > >> > 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