Discussion:
Proposal : REINDEX SCHEMA
Sawada Masahiko
2014-10-12 13:58:24 UTC
Permalink
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
Alvaro Herrera
2014-10-12 15:44:46 UTC
Permalink
Post by Sawada Masahiko
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-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Stephen Frost
2014-10-12 17:27:25 UTC
Permalink
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Fabrízio de Royes Mello
2014-10-13 04:25:47 UTC
Permalink
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Post by Stephen Frost
Post by Alvaro Herrera
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
2014-10-13 08:39:45 UTC
Permalink
On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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..
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.
Post by Fabrízio de Royes Mello
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?
Post by Fabrízio de Royes Mello
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.
Post by Fabrízio de Royes Mello
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.
Post by Fabrízio de Royes Mello
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-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabrízio de Royes Mello
2014-10-13 13:23:13 UTC
Permalink
On Mon, Oct 13, 2014 at 1:25 PM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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..
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.
Post by Fabrízio de Royes Mello
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?
Post by Fabrízio de Royes Mello
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.
Post by Fabrízio de Royes Mello
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.
Post by Fabrízio de Royes Mello
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
Post by Fabrízio de Royes Mello
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
Robert Haas
2014-10-13 14:16:55 UTC
Permalink
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Sawada Masahiko
2014-10-15 14:41:12 UTC
Permalink
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Fabrízio de Royes Mello
2014-10-16 19:32:58 UTC
Permalink
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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);
Post by Sawada Masahiko
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
Post by Sawada Masahiko
Post by Robert Haas
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
2014-10-19 15:02:47 UTC
Permalink
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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
* 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;
Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
/* 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);
Post by Sawada Masahiko
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
Fabrízio de Royes Mello
2014-10-20 00:37:38 UTC
Permalink
Post by Sawada Masahiko
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Post by Sawada Masahiko
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
database,
but we can not do reindexing per schema for now.
It seems doubtful that there really is much use for this feature,
but
Post by Sawada Masahiko
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
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.
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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
* 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;
Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
/* 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 */
get_namespace_oid(objectName,
Post by Sawada Masahiko
Post by Fabrízio de Royes Mello
false);
Post by Sawada Masahiko
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!
Post by Sawada Masahiko
I agree 2) - 5).
:-)
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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
Post by Sawada Masahiko
Post by Fabrízio de Royes Mello
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
Fabrízio de Royes Mello
2014-10-20 00:41:16 UTC
Permalink
On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello <
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko <
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
database,
but we can not do reindexing per schema for now.
It seems doubtful that there really is much use for this
feature, but
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
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.
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Sawada Masahiko
feature
1) Compile without warnings
* 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;
Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
/* 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 */
get_namespace_oid(objectName,
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
false);
Post by Sawada Masahiko
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!
Post by Sawada Masahiko
I agree 2) - 5).
:-)
Post by Sawada Masahiko
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
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
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
2014-10-20 13:24:53 UTC
Permalink
On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello
On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko
Post by Sawada Masahiko
Post by Robert Haas
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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.
Post by Sawada Masahiko
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
* 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;
Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
/* 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 */
get_namespace_oid(objectName,
false);
Post by Sawada Masahiko
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-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabrízio de Royes Mello
2014-10-20 14:01:25 UTC
Permalink
On Mon, Oct 20, 2014 at 9:41 AM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
On Sun, Oct 19, 2014 at 10:37 PM, Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
On Fri, Oct 17, 2014 at 4:32 AM, Fabrízio de Royes Mello
On Wed, Oct 15, 2014 at 11:41 AM, Sawada Masahiko
On Mon, Oct 13, 2014 at 11:16 PM, Robert Haas <
On Sun, Oct 12, 2014 at 1:27 PM, Stephen Frost <
Post by Stephen Frost
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Post by Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
Post by Stephen Frost
/
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
Post by Fabrízio de Royes Mello
Post by Fabrízio de Royes Mello
Post by Sawada Masahiko
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
* 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;
Assert(kind == OBJECT_DATABASE || kind == OBJECT_SCHEMA);
/* 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 */
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!
Post by Sawada Masahiko
I agree 2) - 5).
:-)
Post by Sawada Masahiko
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
Post by Fabrízio de Royes Mello
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
Alvaro Herrera
2014-10-22 23:21:21 UTC
Permalink
Post by Sawada Masahiko
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-***@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Fabrízio de Royes Mello
2014-10-23 18:41:05 UTC
Permalink
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Post by Alvaro Herrera
Post by Sawada Masahiko
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
Loading...