Re: Proposal : REINDEX SCHEMA

Hi all,

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.
So we must use reindexdb command if we want to do.
This new syntax supports it as SQL command.
This use similar logic as REINDEX DATABASE, but we can use it in
transaction block.
Here is some example,

-- Table information
[postgres][5432](1)=# \d n1.hoge
Table "n1.hoge"
Column | Type | Modifiers
--------+---------+-----------
col | integer | not null
Indexes:
"hoge_pkey" PRIMARY KEY, btree (col)

[postgres][5432](1)=# \d n2.hoge
Table "n2.hoge"
Column | Type | Modifiers
--------+---------+-----------
col | integer |

[postgres][5432](1)=# \d n3.hoge
Did not find any relation named "n3.hoge".

-- Do reindexing
[postgres][5432](1)=# reindex schema n1;
NOTICE: table "n1.hoge" was reindexed
REINDEX
[postgres][5432](1)=# reindex schema n2;
REINDEX
[postgres][5432](1)=# reindex schema n3;
NOTICE: schema"n3" does not hava any table
REINDEX

Please review and comment.

Regards,

-------
Sawada Masahiko

Sawada Masahiko wrote:
> Hi all,
>
> 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.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

Thanks,

Stephen

On Sun, Oct 12, 2014 at 2: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..
>

Some review:

1) +1 to "REINDEX ALL IN SCHEMA name"

2) IMHO the logic should be exactly the same as REINDEX DATABASE, including
the transaction control. Imagine a schema with a lot of tables, you can
lead to a deadlock using just one transaction block.

3) The patch was applied to master and compile without warnings

4) Typo (... does not have any table)

+ if (!reindex_schema(heapOid))
+ ereport(NOTICE,
+ (errmsg("schema\"%s\" does not hava any table",
+ schema->relname)));

5) Missing of regression tests, please add it to
src/test/regress/sql/create_index.sql

6) You need to add psql complete tabs

7) I think we can add "-S / --schema" option do reindexdb in this patch
too. What do you think?

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

On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Sun, Oct 12, 2014 at 2: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..
>>
>
> Some review:
>
> 1) +1 to "REINDEX ALL IN SCHEMA name"
>

Thank you for comment and reviewing!

I agree with this.
I will change syntax to above like that.

>
> 2) IMHO the logic should be exactly the same as REINDEX DATABASE, including
> the transaction control. Imagine a schema with a lot of tables, you can lead
> to a deadlock using just one transaction block.
>

Yep, it should be same as REINDEX DATABASE.
IMO, we can put them into one function if they use exactly same logic.
Thought?

>
> 3) The patch was applied to master and compile without warnings
>
>
> 4) Typo (... does not have any table)
>
> + if (!reindex_schema(heapOid))
> + ereport(NOTICE,
> + (errmsg("schema\"%s\" does not hava any table",
> + schema->relname)));
>

Thanks! I will correct typo.

>
> 5) Missing of regression tests, please add it to
> src/test/regress/sql/create_index.sql
>
> 6) You need to add psql complete tabs
>

Next version patch, that I will submit, will have 5), 6) things you pointed.

> 7) I think we can add "-S / --schema" option do reindexdb in this patch too.
> What do you think?
>

+1 to add "-S / --schema" option
I was misunderstanding about that.
I will make the patch which adds this option as separate patch.

--
Regards,

-------
Sawada Masahiko

On Mon, Oct 13, 2014 at 5:39 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> > On Sun, Oct 12, 2014 at 2: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..
> >>
> >
> > Some review:
> >
> > 1) +1 to "REINDEX ALL IN SCHEMA name"
> >
>
> Thank you for comment and reviewing!
>
> I agree with this.
> I will change syntax to above like that.
>
> >
> > 2) IMHO the logic should be exactly the same as REINDEX DATABASE,
including
> > the transaction control. Imagine a schema with a lot of tables, you can
lead
> > to a deadlock using just one transaction block.
> >
>
> Yep, it should be same as REINDEX DATABASE.
> IMO, we can put them into one function if they use exactly same logic.
> Thought?
>
> >
> > 3) The patch was applied to master and compile without warnings
> >
> >
> > 4) Typo (... does not have any table)
> >
> > + if (!reindex_schema(heapOid))
> > + ereport(NOTICE,
> > + (errmsg("schema\"%s\" does not hava any table",
> > + schema->relname)));
> >
>
> Thanks! I will correct typo.
>
> >
> > 5) Missing of regression tests, please add it to
> > src/test/regress/sql/create_index.sql
> >
> > 6) You need to add psql complete tabs
> >
>
> Next version patch, that I will submit, will have 5), 6) things you
pointed.
>
> > 7) I think we can add "-S / --schema" option do reindexdb in this patch
too.
> > What do you think?
> >
>
> +1 to add "-S / --schema" option
> I was misunderstanding about that.
> I will make the patch which adds this option as separate patch.
>

I registered to the next commitfest [1] and put myself as reviewer.

Regards,

[1] https://commitfest.postgresql.org/action/patch_view?id=1598

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

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.
I changed syntax to REINDEX ALL IN SCHEMA, but I felt a sense of
discomfort a little
as Robert mentioned.

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
001_Add_schema_option_to_reindexdb_v1.patch : It contains reindexdb
"-S/--schema" supporting

Please review and comments.

Regards,

-------
Sawada Masahiko

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

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

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.

> Also, I noticed I had forgotten to add the patch regarding document of
> reindexdb.
>

Yeah... I forgot it too... :-) I'm not a native speaker but IMHO the docs
is fine.

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

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.

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

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

On Mon, Oct 20, 2014 at 11:24 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> 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!

You're welcome!

> I also did successfully.
> It looks good to me :)
>

Changed status to "Ready for commiter".

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

Sawada Masahiko wrote:

> 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 don't use pg_catalog in the regression test. That way we will
need to update the expected file whenever a new catalog is added, which
seems pointless. Maybe create a schema with a couple of tables
specifically for this, instead.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:
>
> Sawada Masahiko wrote:
>
> > 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 don't use pg_catalog in the regression test. That way we will
> need to update the expected file whenever a new catalog is added, which
> seems pointless. Maybe create a schema with a couple of tables
> specifically for this, instead.
>

Attached new regression test.

Isn't better join the two patches in just one?

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

On Fri, Oct 24, 2014 at 3:41 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
> wrote:
>>
>> Sawada Masahiko wrote:
>>
>> > 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 don't use pg_catalog in the regression test. That way we will
>> need to update the expected file whenever a new catalog is added, which
>> seems pointless. Maybe create a schema with a couple of tables
>> specifically for this, instead.
>>
>
> Attached new regression test.

Hunk #1 FAILED at 1.
1 out of 2 hunks FAILED -- saving rejects to file
src/bin/scripts/t/090_reindexdb.pl.rej

I tried to apply the 001 patch after applying the 000, but it was not
applied cleanly.

At least to me, it's more intuitive to use "SCHEMA" instead of "ALL IN SCHEMA"
here because we already use "DATABASE" instead of "ALL IN DATABASE".

Regards,

--
Fujii Masao

On Thu, Nov 6, 2014 at 8:00 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Oct 24, 2014 at 3:41 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>> On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
>> wrote:
>>>
>>> Sawada Masahiko wrote:
>>>
>>> > 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 don't use pg_catalog in the regression test. That way we will
>>> need to update the expected file whenever a new catalog is added, which
>>> seems pointless. Maybe create a schema with a couple of tables
>>> specifically for this, instead.
>>>
>>
>> Attached new regression test.
>
> Hunk #1 FAILED at 1.
> 1 out of 2 hunks FAILED -- saving rejects to file
> src/bin/scripts/t/090_reindexdb.pl.rej
>
> I tried to apply the 001 patch after applying the 000, but it was not
> applied cleanly.
>
> At least to me, it's more intuitive to use "SCHEMA" instead of "ALL IN SCHEMA"
> here because we already use "DATABASE" instead of "ALL IN DATABASE".
>

Thank you for reporting.

Um, that's one way of looking at it
I think It's not quite new syntax, and we have not used "ALL IN"
clause in REINDEX command by now.
If we add SQL command to reindex table of all in table space as newly syntax,
we might be able to name it "REINDEX TABLE ALL IN TABLESPACE".

Anyway, I created patch of "REINDEX SCHEMA" version, and attached.
Also inapplicable problem has been solved.

Regards,

-------
Sawada Masahiko

On 23 October 2014 00:21, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:

>> Attached patch is latest version patch I modified above.
>> Also, I noticed I had forgotten to add the patch regarding document of
>> reindexdb.
>
> Please don't use pg_catalog in the regression test. That way we will
> need to update the expected file whenever a new catalog is added, which
> seems pointless. Maybe create a schema with a couple of tables
> specifically for this, instead.

These patches look fine to me. Don't see anybody objecting either.

Are you looking to commit them, or shall I?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 23 October 2014 00:21, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
>>> Attached patch is latest version patch I modified above.
>>> Also, I noticed I had forgotten to add the patch regarding document of
>>> reindexdb.
>>
>> Please don't use pg_catalog in the regression test. That way we will
>> need to update the expected file whenever a new catalog is added, which
>> seems pointless. Maybe create a schema with a couple of tables
>> specifically for this, instead.
>
> These patches look fine to me. Don't see anybody objecting either.
>
> Are you looking to commit them, or shall I?

IMO, SCHEMA is just but fine, that's more consistent with the existing
syntax we have for database and tables.

Btw, there is an error in this patch, there are no ACL checks on the
schema for the user doing the REINDEX, so any user is able to perform
a REINDEX on any schema. Here is an example for a given user, let's
say "poo'":
=> create table foo.g (a int);
ERROR: 42501: permission denied for schema foo
LOCATION: aclcheck_error, aclchk.c:3371
=> reindex schema foo ;
NOTICE: 00000: table "foo.c" was reindexed
LOCATION: ReindexDatabaseOrSchema, indexcmds.c:1930
REINDEX
A regression test to check that would be good as well.

Also, ISTM that it is awkward to modify the values of do_user and
do_system like that in ReindexDatabase for two reasons:
1) They should be set in gram.y
2) This patch passes as a new argument of ReindexDatabase the object
type, something that is overlapping with what do_user and do_system
are aimed for. Why not simply defining a new object type
OBJECT_SYSTEM? As this patch introduces a new object category for
REINDEX, this patch seems to be a good occasion to remove the boolean
dance in REINDEX at the cost of having a new object type for the
concept of a system, which would be a way to define the system
catalogs as a whole.

Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.

So, I think that we need to think a bit more here. We are not far from
smth that could be committed, so marking as "Waiting on Author" for
now. Thoughts?
--
Michael

On Wed, Nov 26, 2014 at 3:48 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> On 23 October 2014 00:21, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>
>>>> Attached patch is latest version patch I modified above.
>>>> Also, I noticed I had forgotten to add the patch regarding document of
>>>> reindexdb.
>>>
>>> Please don't use pg_catalog in the regression test. That way we will
>>> need to update the expected file whenever a new catalog is added, which
>>> seems pointless. Maybe create a schema with a couple of tables
>>> specifically for this, instead.
>>
>> These patches look fine to me. Don't see anybody objecting either.
>>
>> Are you looking to commit them, or shall I?
>
> IMO, SCHEMA is just but fine, that's more consistent with the existing
> syntax we have for database and tables.
>
> Btw, there is an error in this patch, there are no ACL checks on the
> schema for the user doing the REINDEX, so any user is able to perform
> a REINDEX on any schema. Here is an example for a given user, let's
> say "poo'":
> => create table foo.g (a int);
> ERROR: 42501: permission denied for schema foo
> LOCATION: aclcheck_error, aclchk.c:3371
> => reindex schema foo ;
> NOTICE: 00000: table "foo.c" was reindexed
> LOCATION: ReindexDatabaseOrSchema, indexcmds.c:1930
> REINDEX
> A regression test to check that would be good as well.
>

Thank you for testing this patch.
It's a bug.
I will fix it and add test case to regression test.

> Also, ISTM that it is awkward to modify the values of do_user and
> do_system like that in ReindexDatabase for two reasons:
> 1) They should be set in gram.y
> 2) This patch passes as a new argument of ReindexDatabase the object
> type, something that is overlapping with what do_user and do_system
> are aimed for. Why not simply defining a new object type
> OBJECT_SYSTEM? As this patch introduces a new object category for
> REINDEX, this patch seems to be a good occasion to remove the boolean
> dance in REINDEX at the cost of having a new object type for the
> concept of a system, which would be a way to define the system
> catalogs as a whole.

+1 to define new something object type and remove do_user and do_system.
But if we add OBJECT_SYSTEM to ObjectType data type,
system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
It's a bit redundant?

> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
> So, I think that we need to think a bit more here. We are not far from
> smth that could be committed, so marking as "Waiting on Author" for
> now. Thoughts?

Is the table also kind of "object"?

Regards,

-------
Sawada Masahiko

On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> +1 to define new something object type and remove do_user and do_system.
> But if we add OBJECT_SYSTEM to ObjectType data type,
> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
> It's a bit redundant?
Yes, kind of. That's a superset of a type of relations, aka a set of
catalog tables. If you find something cleaner to propose, feel free.

>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
>> So, I think that we need to think a bit more here. We are not far from
>> smth that could be committed, so marking as "Waiting on Author" for
>> now. Thoughts?
>
> Is the table also kind of "object"?
Sorry, I am not sure I follow you here. Indexes and tables have
already their relkind set in ReindexStmt, and I think that we're fine
to continue letting them go in their own reindex code path for now.
--
Michael

On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> +1 to define new something object type and remove do_user and do_system.
>> But if we add OBJECT_SYSTEM to ObjectType data type,
>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
>> It's a bit redundant?
> Yes, kind of. That's a superset of a type of relations, aka a set of
> catalog tables. If you find something cleaner to propose, feel free.

I thought we can add new struct like ReindexObjectType which has
REINDEX_OBJECT_TABLE,
REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax.

>>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
>>> So, I think that we need to think a bit more here. We are not far from
>>> smth that could be committed, so marking as "Waiting on Author" for
>>> now. Thoughts?
>>
>> Is the table also kind of "object"?
> Sorry, I am not sure I follow you here. Indexes and tables have
> already their relkind set in ReindexStmt, and I think that we're fine
> to continue letting them go in their own reindex code path for now.

It was not enough, sorry.
I mean that there is already ReindexTable() function.
if we renamed ReindexObject, I would feel uncomfortable feeling.
Because table is also kind of "object".

Regards,

-------
Sawada Masahiko

On Thu, Nov 27, 2014 at 8:18 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> +1 to define new something object type and remove do_user and do_system.
>>> But if we add OBJECT_SYSTEM to ObjectType data type,
>>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
>>> It's a bit redundant?
>> Yes, kind of. That's a superset of a type of relations, aka a set of
>> catalog tables. If you find something cleaner to propose, feel free.
>
> I thought we can add new struct like ReindexObjectType which has
> REINDEX_OBJECT_TABLE,
> REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax.
Check.

>>>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
>>>> So, I think that we need to think a bit more here. We are not far from
>>>> smth that could be committed, so marking as "Waiting on Author" for
>>>> now. Thoughts?
>>>
>>> Is the table also kind of "object"?
>> Sorry, I am not sure I follow you here. Indexes and tables have
>> already their relkind set in ReindexStmt, and I think that we're fine
>> to continue letting them go in their own reindex code path for now.
>
> It was not enough, sorry.
> I mean that there is already ReindexTable() function.
> if we renamed ReindexObject, I would feel uncomfortable feeling.
> Because table is also kind of "object".
Check.

If you get that done, I'll have an extra look at it and then let's
have a committer look at it.
Regards,
--
Michael

On Thu, Nov 27, 2014 at 11:43 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Nov 27, 2014 at 8:18 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Nov 27, 2014 at 1:07 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Thu, Nov 27, 2014 at 12:55 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> +1 to define new something object type and remove do_user and do_system.
>>>> But if we add OBJECT_SYSTEM to ObjectType data type,
>>>> system catalogs are OBJECT_SYSTEM as well as OBJECT_TABLE.
>>>> It's a bit redundant?
>>> Yes, kind of. That's a superset of a type of relations, aka a set of
>>> catalog tables. If you find something cleaner to propose, feel free.
>>
>> I thought we can add new struct like ReindexObjectType which has
>> REINDEX_OBJECT_TABLE,
>> REINDEX_OBJECT_SYSTEM and so on. It's similar to GRANT syntax.
> Check.
>
>>>>> Another thing, ReindexDatabaseOrSchema should be renamed to ReindexObject.
>>>>> So, I think that we need to think a bit more here. We are not far from
>>>>> smth that could be committed, so marking as "Waiting on Author" for
>>>>> now. Thoughts?
>>>>
>>>> Is the table also kind of "object"?
>>> Sorry, I am not sure I follow you here. Indexes and tables have
>>> already their relkind set in ReindexStmt, and I think that we're fine
>>> to continue letting them go in their own reindex code path for now.
>>
>> It was not enough, sorry.
>> I mean that there is already ReindexTable() function.
>> if we renamed ReindexObject, I would feel uncomfortable feeling.
>> Because table is also kind of "object".
> Check.
>
> If you get that done, I'll have an extra look at it and then let's
> have a committer look at it.

Attached patch is latest version.
I added new enum values like REINDEX_OBJECT_XXX,
and changed ReindexObject() function.
Also ACL problem is fixed for this version.

Regards,

-------
Sawada Masahiko

On Mon, Dec 1, 2014 at 11:29 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Nov 27, 2014 at 11:43 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> If you get that done, I'll have an extra look at it and then let's
>> have a committer look at it.
>
> Attached patch is latest version.
> I added new enum values like REINDEX_OBJECT_XXX,
> and changed ReindexObject() function.
> Also ACL problem is fixed for this version.

Thanks for the updated version.
I have been looking at this patch more deeply, and I noticed a couple
of things that were missing or mishandled:
- The patch completely ignored that a catalog schema could be reindexed
- When reindexing pg_catalog as a schema, it is critical to reindex
pg_class first as reindex_relation updates pg_class.
- gram.y generated some warnings because ReindexObjectType stuff was
casted as ObjectType.
- There was a memory leak, the scan keys palloc'd in ReindexObject
were not pfree'd
- Using do_user, do_system and now do_database was really confusing
and reduced code lisibility. I reworked it using the object kind
- The patch to support SCHEMA in reindexdb has been forgotten.
Reattaching it here.
Adding on top of that a couple of things cleaned up, like docs and
typos, and I got the patch attached. Let's have a committer have a
look a it now, I am marking that as "Ready for Committer".
Regards,
--
Michael

On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Adding on top of that a couple of things cleaned up, like docs and
> typos, and I got the patch attached. Let's have a committer have a
> look a it now, I am marking that as "Ready for Committer".
For the archives, this has been committed as fe263d1. Thanks Simon for
looking and the final push. And sorry that I didn't spot the issue
with tap tests when reviewing, check-world passed but my dev VM missed
necessary perl packages.
Regards,
--
Michael

On Tue, Dec 9, 2014 at 10:10 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> Adding on top of that a couple of things cleaned up, like docs and
>> typos, and I got the patch attached. Let's have a committer have a
>> look a it now, I am marking that as "Ready for Committer".
> For the archives, this has been committed as fe263d1. Thanks Simon for
> looking and the final push. And sorry that I didn't spot the issue
> with tap tests when reviewing, check-world passed but my dev VM missed
> necessary perl packages.
While re-looking at that. I just found that when selecting the
relations that are reindexed for a schema we ignore materialized view
as the key scan is only done using 'r' as relkind. The patch attached
fixes that.
Thanks,
--
Michael

On Tue, Dec 9, 2014 at 4:44 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 9, 2014 at 10:10 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Tue, Dec 2, 2014 at 3:42 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> Adding on top of that a couple of things cleaned up, like docs and
>>> typos, and I got the patch attached. Let's have a committer have a
>>> look a it now, I am marking that as "Ready for Committer".
>> For the archives, this has been committed as fe263d1. Thanks Simon for
>> looking and the final push. And sorry that I didn't spot the issue
>> with tap tests when reviewing, check-world passed but my dev VM missed
>> necessary perl packages.
> While re-looking at that. I just found that when selecting the
> relations that are reindexed for a schema we ignore materialized view
> as the key scan is only done using 'r' as relkind. The patch attached
> fixes that.
Here is an updated patch doing as well that:
- Regression test checking if user has permissions on schema was broken
- Silent NOTICE messages of REINDEX by having client_min_messages set
to WARINING (thoughts about having a plpgsql function doing
consistency checks of relfilenode before and after reindex?)
--
Michael

On 9 December 2014 at 17:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

>> While re-looking at that. I just found that when selecting the
>> relations that are reindexed for a schema we ignore materialized view
>> as the key scan is only done using 'r' as relkind. The patch attached
>> fixes that.
> Here is an updated patch doing as well that:
> - Regression test checking if user has permissions on schema was broken
> - Silent NOTICE messages of REINDEX by having client_min_messages set
> to WARINING (thoughts about having a plpgsql function doing
> consistency checks of relfilenode before and after reindex?)

ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
the way it issues NOTICE messages.

I'm inclined to simply remove the NOTICE messages, except when a
REINDEX ... VERBOSE is requested.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tuesday, December 9, 2014, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 9 December 2014 at 17:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> <javascript:;>> wrote:
>
> >> While re-looking at that. I just found that when selecting the
> >> relations that are reindexed for a schema we ignore materialized view
> >> as the key scan is only done using 'r' as relkind. The patch attached
> >> fixes that.
> > Here is an updated patch doing as well that:
> > - Regression test checking if user has permissions on schema was broken
> > - Silent NOTICE messages of REINDEX by having client_min_messages set
> > to WARINING (thoughts about having a plpgsql function doing
> > consistency checks of relfilenode before and after reindex?)
>
> ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
> the way it issues NOTICE messages.
>
> I'm inclined to simply remove the NOTICE messages, except when a
> REINDEX ... VERBOSE is requested.
>
>
+1 to remove the NOTICE messages except when specifying VERBOSE.

It would output a lot of messages if there are many table in schema.
If nobody objects to it, I will work on this.

Regards,

--
Sawada Masahiko

--
Regards,

-------
Sawada Masahiko

On Tue, Dec 9, 2014 at 6:00 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 9 December 2014 at 17:17, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
>>> While re-looking at that. I just found that when selecting the
>>> relations that are reindexed for a schema we ignore materialized view
>>> as the key scan is only done using 'r' as relkind. The patch attached
>>> fixes that.
>> Here is an updated patch doing as well that:
>> - Regression test checking if user has permissions on schema was broken
>> - Silent NOTICE messages of REINDEX by having client_min_messages set
>> to WARINING (thoughts about having a plpgsql function doing
>> consistency checks of relfilenode before and after reindex?)
>
> ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
> the way it issues NOTICE messages.
>
> I'm inclined to simply remove the NOTICE messages, except when a
> REINDEX ... VERBOSE is requested.
OK. Perhaps that's not worth mentioning in the release notes, but some
users may be used to the old behavior. What about the other issues
(regression test for permissions incorrect and matviews)?
--
Michael

On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> OK. Perhaps that's not worth mentioning in the release notes, but some
> users may be used to the old behavior. What about the other issues
> (regression test for permissions incorrect and matviews)?
Here is in any case an updated patch to fix all those things rebased
on latest HEAD (ae4e688).
Thanks,
--
Michael

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
> the way it issues NOTICE messages.

> I'm inclined to simply remove the NOTICE messages, except when a
> REINDEX ... VERBOSE is requested.

My recollection is that those other commands do issue messages always,
but at some DEBUGn log level when not VERBOSE. Ideally REINDEX would
adopt that same behavior.

regards, tom lane

On Tue, Dec 9, 2014 at 11:59 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> ISTM that REINDEX is not consistent with VACUUM, ANALYZE or CLUSTER in
>> the way it issues NOTICE messages.
>
>> I'm inclined to simply remove the NOTICE messages, except when a
>> REINDEX ... VERBOSE is requested.
>
> My recollection is that those other commands do issue messages always,
> but at some DEBUGn log level when not VERBOSE. Ideally REINDEX would
> adopt that same behavior.
>

So it seems to that REINDEX command issues messages as INFO level when
that is specified VERBOSE option.
If not specified, it issue message as DEBUG2.

Regards,

-------
Sawada Masahiko

On Tue, Dec 9, 2014 at 10:21 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > OK. Perhaps that's not worth mentioning in the release notes, but some
> > users may be used to the old behavior. What about the other issues
> > (regression test for permissions incorrect and matviews)?
> Here is in any case an updated patch to fix all those things rebased
> on latest HEAD (ae4e688).
>

The patch is fine:
- No compiler warnings
- Added properly regressions tests and run ok

There are no changes in the docs. Maybe we can mention matviews on REINDEX
SCHEMA docs, what do you think?

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

On Wed, Dec 10, 2014 at 1:37 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Tue, Dec 9, 2014 at 10:21 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>>
>> On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > OK. Perhaps that's not worth mentioning in the release notes, but some
>> > users may be used to the old behavior. What about the other issues
>> > (regression test for permissions incorrect and matviews)?
>> Here is in any case an updated patch to fix all those things rebased
>> on latest HEAD (ae4e688).
>>
>
> The patch is fine:
> - No compiler warnings
> - Added properly regressions tests and run ok
>
> There are no changes in the docs. Maybe we can mention matviews on REINDEX
> SCHEMA docs, what do you think?
Current documentation tells that all the indexes in schema are
reindexed, only matviews and relations can have one, so that's
implicitly specified IMO. If in the future we add support for another
relkind and that it can have indexes, we won't need to update the docs
as well.
--
Michael

On 9 December 2014 at 12:21, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> OK. Perhaps that's not worth mentioning in the release notes, but some
>> users may be used to the old behavior. What about the other issues
>> (regression test for permissions incorrect and matviews)?
> Here is in any case an updated patch to fix all those things rebased
> on latest HEAD (ae4e688).
> Thanks,

I rewrote and applied this patch to ensure we didn't introduce further bugs.

Tests for the reindex part were rewritten from scratch also.

Rather unluckily that seems to have coincided with Tom's changes, so
I've only added the bits that have nothing to do with Tom's changes -
all of which stand.

(I just got going again after my flight back from Tokyo).

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Dec 12, 2014 at 8:00 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 9 December 2014 at 12:21, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> > On Tue, Dec 9, 2014 at 6:43 PM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> OK. Perhaps that's not worth mentioning in the release notes, but some
> >> users may be used to the old behavior. What about the other issues
> >> (regression test for permissions incorrect and matviews)?
> > Here is in any case an updated patch to fix all those things rebased
> > on latest HEAD (ae4e688).
> > Thanks,
>
> I rewrote and applied this patch to ensure we didn't introduce further
> bugs.
>
> Tests for the reindex part were rewritten from scratch also.
>
> Rather unluckily that seems to have coincided with Tom's changes, so
> I've only added the bits that have nothing to do with Tom's changes -
> all of which stand.
>

Glad that things are in order now. I have nothing else left to complain
about with this feature. Thanks.

(I just got going again after my flight back from Tokyo).
>
I understand going east makes one day longer :)
--
Michael

On Fri, Dec 12, 2014 at 8:41 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Fri, Dec 12, 2014 at 8:00 AM, Simon Riggs <simon(at)2ndquadrant(dot)com>
> wrote:
>
>> Rather unluckily that seems to have coincided with Tom's changes, so
>> I've only added the bits that have nothing to do with Tom's changes -
>> all of which stand.
>>
> Glad that things are in order now. I have nothing else left to complain
> about with this feature. Thanks.
>
Actually there is one thing left: this patch from Sawada-san to finish the
cleanup of ReindexStmt for the boolean flags.
http://www.postgresql.org/message-id/CAD21AoCXFE1J+hSkbeJ80rqqnhR8m_YUxdGKwZ4dL8zPqT8gjg@mail.gmail.com

Regards,
--
Michael

On 11 December 2014 at 23:41, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> Glad that things are in order now. I have nothing else left to complain
> about with this feature. Thanks.

I think you've discovered a new meaning of Ready for Committer

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Fri, Dec 12, 2014 at 10:19 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> On 11 December 2014 at 23:41, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>
> > Glad that things are in order now. I have nothing else left to complain
> > about with this feature. Thanks.
>
> I think you've discovered a new meaning of Ready for Committer
>
Yep. Sorry for missing so many things.
--
Michael