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

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.
I agree 2) - 5).
Attached patch is latest version patch I modified above.
Also, I noticed I had forgotten to add the patch regarding document of
reindexdb.

Please review and comments.

Regards,

-------
Sawada Masahiko