Re: Proposal : REINDEX SCHEMA

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

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