Re: Proposal : REINDEX SCHEMA - Mailing list pgsql-hackers
| From | Fabrízio de Royes Mello |
|---|---|
| Subject | Re: Proposal : REINDEX SCHEMA |
| Date | |
| Msg-id | CAFcNs+q8DB9YpFwZ8GiY1i7TUuLgapRKCRisiGtFTemytA3=jg@mail.gmail.com Whole thread Raw |
| In response to | Re: Proposal : REINDEX SCHEMA (Sawada Masahiko <sawada.mshk@gmail.com>) |
| Responses |
Re: Proposal : REINDEX SCHEMA
|
| List | pgsql-hackers |
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.
>
> 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... :-)
> 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
> 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
> "-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
pgsql-hackers by date: