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
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
