Proposal : REINDEX SCHEMA

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

Thanks,

Stephen

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

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

Show quoted text

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@gmail.com> wrote:

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 13, 2014 at 5:39 AM, Sawada Masahiko <sawada.mshk@gmail.com>
wrote:

On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

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

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]https://commitfest.postgresql.org/action/patch_view?id=1598 and put myself as reviewer.

Regards,

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

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

Show quoted text

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

On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com>
wrote:

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.

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

Show quoted text

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@gmail.com> wrote:

On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com>

wrote:

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.

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

Show quoted text

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@gmail.com> wrote:

On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com>
wrote:

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.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Oct 20, 2014 at 11:24 AM, Sawada Masahiko <sawada.mshk@gmail.com>
wrote:

On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Sun, Oct 19, 2014 at 1:02 PM, Sawada Masahiko <sawada.mshk@gmail.com

wrote:

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.

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

Show quoted text

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.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

Show quoted text

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@gmail.com> wrote:

On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Nov 6, 2014 at 8:00 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, Oct 24, 2014 at 3:41 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Wed, Oct 22, 2014 at 9:21 PM, Alvaro Herrera <alvherre@2ndquadrant.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@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 19, 2014 at 1:37 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 23 October 2014 00:21, Alvaro Herrera <alvherre@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers