Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 14, 2015 at 4:30 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Thu, May 14, 2015 at 9:58 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko sawada.m...@gmail.com wrote: The v15 patch emits a line for each table when reindexing multiple tables, and emits a line for each index when reindexing single table. But v14 patch emits a line for each index, regardless of reindex target. Should I change back to v14 patch? Uh, maybe. What made you change it? I thought that the users who want to reindex multiple tables are interested in the time to reindex whole table takes. But I think it seems sensible to emit a line for each index even when reindex multiple tables. The v16 patch is based on v14 and a few modified is attached. Thanks for updating the patch! The regression test failed because you forgot to remove the trailng period from the verbose message in the expected file of the regression test. I just fixed it and push the patch. 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Sun, May 10, 2015 at 2:23 AM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a
fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a
bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation
regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
I applied the two patches to master and I got some errors when compile:
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in
this function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only
once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from
tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 14, 2015 at 9:58 AM, Robert Haas robertmh...@gmail.com wrote:
On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
The v15 patch emits a line for each table when reindexing multiple
tables, and emits a line for each index when reindexing single table.
But v14 patch emits a line for each index, regardless of reindex target.
Should I change back to v14 patch?
Uh, maybe. What made you change it?
I thought that the users who want to reindex multiple tables are
interested in the time to reindex whole table takes.
But I think it seems sensible to emit a line for each index even when
reindex multiple tables.
The v16 patch is based on v14 and a few modified is attached.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 998340c..703b760 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable
+REINDEX [ ( { VERBOSE } [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable
/synopsis
/refsynopsisdiv
@@ -150,6 +150,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8c8a9ea..bac9fbe 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3184,13 +3185,17 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+int options)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3334,6 +3339,14 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ if (options REINDEXOPT_VERBOSE)
+ ereport(INFO,
+(errmsg(index \%s\ was reindexed,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3375,7 +3388,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid relid, int flags, int options)
{
Relation rel;
Oid toast_relid;
@@ -3466,7 +3479,7 @@ reindex_relation(Oid relid, int flags)
RelationSetIndexList(rel, doneIndexes, InvalidOid);
reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS),
- persistence);
+ persistence, options);
CommandCounterIncrement();
@@ -3501,7 +3514,7 @@ reindex_relation(Oid relid, int flags)
* still hold the lock on the master table.
*/
if ((flags REINDEX_REL_PROCESS_TOAST) OidIsValid(toast_relid))
- result |= reindex_relation(toast_relid, flags);
+ result |= reindex_relation(toast_relid, flags, options);
return result;
}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 3febdd5..7ab4874 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1532,7 +1532,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
- reindex_relation(OIDOldHeap, reindex_flags);
+ reindex_relation(OIDOldHeap, reindex_flags, 0);
/*
* If the relation being rebuild is pg_class, swap_relation_files()
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 351d48e..7340a1f 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1681,7 +1681,7 @@ ChooseIndexColumnNames(List *indexElems)
* Recreate a specific index.
*/
Oid
-ReindexIndex(RangeVar *indexRelation)
+ReindexIndex(RangeVar *indexRelation, int options)
{
Oid indOid;
Oid heapOid = InvalidOid;
@@ -1706,7 +1706,7 @@ ReindexIndex(RangeVar *indexRelation)
persistence = irel-rd_rel-relpersistence;
index_close(irel, NoLock);
-
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Robert Haas wrote: On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any good reason to make this different. I was misremembering the INFO situation. Here's one item in the archives I found in a very quick search, which says that INFO is the right thing to use: http://www.postgresql.org/message-id/24874.1231183...@sss.pgh.pa.us Cheers -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any good reason to make this different. Also, when multiple tables are reindexed, do we emit lines for each index, or only for each table? If we're going to emit a line for each index in the single-table mode, it seems more sensible to do the same for the multi-table forms also. Hmm, yeah, I agree with that. I thought the patch worked that way, but I see now that it doesn't. I think that should be changed. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 14, 2015 at 3:10 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? I think it should be INFO level because it is a information of REINDEX command,such as progress of itself, CPU usage and so on. it would be overkill if we output the logs as NOTICE level, and verbose outputs of other maintenance command are emitted as INFO level. Also, when multiple tables are reindexed, do we emit lines for each index, or only for each table? If we're going to emit a line for each index in the single-table mode, it seems more sensible to do the same for the multi-table forms also. I agree that we emit lines for each table when we do reindex multiple tables. The latest patch is attached. Regards, --- Sawada Masahiko 000_reindex_verbose_v15.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 14, 2015 at 4:53 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any good reason to make this different. Also, when multiple tables are reindexed, do we emit lines for each index, or only for each table? If we're going to emit a line for each index in the single-table mode, it seems more sensible to do the same for the multi-table forms also. Hmm, yeah, I agree with that. I thought the patch worked that way, but I see now that it doesn't. I think that should be changed. The v15 patch emits a line for each table when reindexing multiple tables, and emits a line for each index when reindexing single table. But v14 patch emits a line for each index, regardless of reindex target. Should I change back to v14 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, May 13, 2015 at 2:49 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Thu, May 14, 2015 at 12:28 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Sorry, I forgot attach files. Review comments: - Customarily we use int, rather than uint8, for flags variables. I think we should do that here also. - reindex_index() emits a log message either way, but at DEBUG2 level without VERBOSE and at the INFO level with it. I think we should skip it altogether without VERBOSE. i.e. if (options REINDEXOPT_VERBOSE) ereport(...) - The errmsg() in that function should not end with a period. - The 000 patch makes a pointless whitespace change to tab-complete.c. - Instead of an enumerated type (ReindexOption) just use #define to define the flag value. Apart from those fairly minor issues, I think this looks pretty solid. Thank you for reviwing.. All fixed. IMHO we don't need pg_rusage_init(ru0) if the verbose options is not setted. Maybe change: + +pg_rusage_init(ru0); to + +if (options REINDEXOPT_VERBOSE) +pg_rusage_init(ru0); 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Sorry, I forgot attach files. Review comments: - Customarily we use int, rather than uint8, for flags variables. I think we should do that here also. - reindex_index() emits a log message either way, but at DEBUG2 level without VERBOSE and at the INFO level with it. I think we should skip it altogether without VERBOSE. i.e. if (options REINDEXOPT_VERBOSE) ereport(...) - The errmsg() in that function should not end with a period. - The 000 patch makes a pointless whitespace change to tab-complete.c. - Instead of an enumerated type (ReindexOption) just use #define to define the flag value. Apart from those fairly minor issues, I think this looks pretty solid. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko sawada.m...@gmail.com wrote: The v15 patch emits a line for each table when reindexing multiple tables, and emits a line for each index when reindexing single table. But v14 patch emits a line for each index, regardless of reindex target. Should I change back to v14 patch? Uh, maybe. What made you change it? -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 14, 2015 at 12:28 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Sorry, I forgot attach files. Review comments: - Customarily we use int, rather than uint8, for flags variables. I think we should do that here also. - reindex_index() emits a log message either way, but at DEBUG2 level without VERBOSE and at the INFO level with it. I think we should skip it altogether without VERBOSE. i.e. if (options REINDEXOPT_VERBOSE) ereport(...) - The errmsg() in that function should not end with a period. - The 000 patch makes a pointless whitespace change to tab-complete.c. - Instead of an enumerated type (ReindexOption) just use #define to define the flag value. Apart from those fairly minor issues, I think this looks pretty solid. Thank you for reviwing.. All fixed. Regards, --- Sawada Masahiko 000_reindex_verbose_v14.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? Also, when multiple tables are reindexed, do we emit lines for each index, or only for each table? If we're going to emit a line for each index in the single-table mode, it seems more sensible to do the same for the multi-table forms also. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a
fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a
bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation
regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
I applied the two patches to master and I got some errors when compile:
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in
this function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only
once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from
tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3335,7 +3335,6 @@ psql_completion(const char *text, int
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Sun, May 10, 2015 at 2:23 AM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a
fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a
bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation
regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
I applied the two patches to master and I got some errors when compile:
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in
this function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only
once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from
tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a
fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a
bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation
regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
I applied the two patches to master and I got some errors when compile:
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression
has no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression
has no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression
has no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression
has no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression
has no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in
this function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only
once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from
tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int
end)
/* REINDEX */
else if (pg_strcasecmp(prev_wd, REINDEX) == 0)
{
- static const
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a
fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
I applied the two patches to master and I got some errors when compile:
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression has
no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in this
function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only
once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from
tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int end)
/* REINDEX */
else if (pg_strcasecmp(prev_wd, REINDEX) == 0)
{
- static const char *const list_REINDEX[] =
{TABLE, INDEX, SYSTEM, SCHEMA, DATABASE, NULL};
COMPLETE_WITH_LIST(list_REINDEX);
The attached fix it and now seems good to me.
Regards,
--
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 5/7/15, Sawada Masahiko sawada.m...@gmail.com wrote:
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko sawada.m...@gmail.com
javascript:; wrote:
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com
javascript:; wrote:
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
sawada.m...@gmail.com
javascript:; wrote:
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
No. The unparenthesized VACUUM syntax was added back before we
realized that that kind of syntax is a terrible idea. It requires
every option to be a keyword, and those keywords have to be in a fixed
order. I believe the intention is to keep the old VACUUM syntax
around for backward-compatibility, but not to extend it. Same for
EXPLAIN and COPY.
REINDEX will have only one option VERBOSE for now.
Even we're in a situation like that it's not clear to be added newly
additional option to REINDEX now, we should need to put parenthesis?
In my opinion, yes. The whole point of a flexible options syntax is
that we can add new options without changing the grammar. That
involves some compromise on the syntax, which doesn't bother me a bit.
Our previous experiments with this for EXPLAIN and COPY and VACUUM
have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar
whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
Also I'm not sure that both implementation and documentation regarding
VERBOSE option should be optional.
I don't know what this means.
Sorry for confusing you.
Please ignore this.
Sorry, I forgot attach files.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 998340c..703b760 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable
+REINDEX [ ( { VERBOSE } [, ...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable
/synopsis
/refsynopsisdiv
@@ -150,6 +150,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index ac3b785..c04b907 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3133,13 +3134,22 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+uint8 options)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
+
+ /* Set option(s) */
+ if (options REINDEXOPT_VERBOSE)
+ elevel = INFO;
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3283,6 +3293,13 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ ereport(elevel,
+ (errmsg(index \%s\ was reindexed.,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3324,7 +3341,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid relid, int flags, uint8 options)
{
Relation rel;
Oid toast_relid;
@@ -3415,7 +3432,7 @@ reindex_relation(Oid relid, int flags)
RelationSetIndexList(rel, doneIndexes, InvalidOid);
reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS),
- persistence);
+ persistence, options);
CommandCounterIncrement();
@@ -3450,7 +3467,7 @@ reindex_relation(Oid relid, int flags)
* still hold
[HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, May 6, 2015 at 5:42 AM, Robert Haas robertmh...@gmail.com javascript:; wrote: On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko sawada.m...@gmail.com javascript:; wrote: On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com javascript:; wrote: On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko sawada.m...@gmail.com javascript:; wrote: VACUUM has both syntax: with parentheses and without parentheses. I think we should have both syntax for REINDEX like VACUUM does because it would be pain to put parentheses whenever we want to do REINDEX. Are the parentheses optional in REINDEX command? No. The unparenthesized VACUUM syntax was added back before we realized that that kind of syntax is a terrible idea. It requires every option to be a keyword, and those keywords have to be in a fixed order. I believe the intention is to keep the old VACUUM syntax around for backward-compatibility, but not to extend it. Same for EXPLAIN and COPY. REINDEX will have only one option VERBOSE for now. Even we're in a situation like that it's not clear to be added newly additional option to REINDEX now, we should need to put parenthesis? In my opinion, yes. The whole point of a flexible options syntax is that we can add new options without changing the grammar. That involves some compromise on the syntax, which doesn't bother me a bit. Our previous experiments with this for EXPLAIN and COPY and VACUUM have worked out quite well, and I see no reason for pessimism here. I agree that flexible option syntax does not need to change grammar whenever we add new options. Attached patch is changed based on your suggestion. And the patch for reindexdb is also attached. Please feedbacks. Also I'm not sure that both implementation and documentation regarding VERBOSE option should be optional. I don't know what this means. Sorry for confusing you. Please ignore this. Regards, --- Sawada Masahiko -- Regards, --- Sawada Masahiko
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko sawada.m...@gmail.com wrote: VACUUM has both syntax: with parentheses and without parentheses. I think we should have both syntax for REINDEX like VACUUM does because it would be pain to put parentheses whenever we want to do REINDEX. Are the parentheses optional in REINDEX command? No. The unparenthesized VACUUM syntax was added back before we realized that that kind of syntax is a terrible idea. It requires every option to be a keyword, and those keywords have to be in a fixed order. I believe the intention is to keep the old VACUUM syntax around for backward-compatibility, but not to extend it. Same for EXPLAIN and COPY. REINDEX will have only one option VERBOSE for now. Even we're in a situation like that it's not clear to be added newly additional option to REINDEX now, we should need to put parenthesis? Also I'm not sure that both implementation and documentation regarding VERBOSE option should be optional. 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Fri, May 1, 2015 at 9:04 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko sawada.m...@gmail.com wrote: VACUUM has both syntax: with parentheses and without parentheses. I think we should have both syntax for REINDEX like VACUUM does because it would be pain to put parentheses whenever we want to do REINDEX. Are the parentheses optional in REINDEX command? No. The unparenthesized VACUUM syntax was added back before we realized that that kind of syntax is a terrible idea. It requires every option to be a keyword, and those keywords have to be in a fixed order. I believe the intention is to keep the old VACUUM syntax around for backward-compatibility, but not to extend it. Same for EXPLAIN and COPY. REINDEX will have only one option VERBOSE for now. Even we're in a situation like that it's not clear to be added newly additional option to REINDEX now, we should need to put parenthesis? In my opinion, yes. The whole point of a flexible options syntax is that we can add new options without changing the grammar. That involves some compromise on the syntax, which doesn't bother me a bit. Our previous experiments with this for EXPLAIN and COPY and VACUUM have worked out quite well, and I see no reason for pessimism here. Also I'm not sure that both implementation and documentation regarding VERBOSE option should be optional. I don't know what this means. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko sawada.m...@gmail.com wrote: VACUUM has both syntax: with parentheses and without parentheses. I think we should have both syntax for REINDEX like VACUUM does because it would be pain to put parentheses whenever we want to do REINDEX. Are the parentheses optional in REINDEX command? No. The unparenthesized VACUUM syntax was added back before we realized that that kind of syntax is a terrible idea. It requires every option to be a keyword, and those keywords have to be in a fixed order. I believe the intention is to keep the old VACUUM syntax around for backward-compatibility, but not to extend it. Same for EXPLAIN and COPY. I agree that it would be nice if the grammar problems could be solved without adding parentheses. But there was a period during which many good ideas for new EXPLAIN options died on the vine because we were using an inextensible syntax for EXPLAIN options. I'm not keen to repeat that experience. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, Apr 10, 2015 at 2:52 AM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Thu, Apr 9, 2015 at 1:14 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko
sawada.m...@gmail.com
wrote:
Thank you for your reviewing.
I modified the patch and attached latest version patch(v7).
Please have a look it.
Looks good to me. Attached patch (v8) just fix a tab indentation in
gram.y.
I had forgotten fix a tab indentation, sorry.
Thank you for reviewing!
It looks good to me too.
Can this patch be marked as Ready for Committer?
+1
Changed status to Ready for Committer.
The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not
added after WITH clause. Did we reach the consensus about this syntax?
The last email from Robert just makes me think that () should be added
into the syntax.
Thank you for taking time for this patch!
I removed the FORCE option from REINDEX, so you would need to update the
patch.
Thanks.
I will change the patch with this change.
This was quite complicated issue since we already have a lot of style
command currently.
We can think many of style from various perspective: kind of DDL, new
or old command, maintenance command. And each command has each its
story.
I believe we have reached the consensus with this style at least once
(please see previous discussion), but we might needs to discuss
more...
Okay, another question is that; WITH must be required whenever the options
are specified? Or should it be abbreviatable?
In previous discussion, the WITH clause is always required by VERBOSE
option. I don't think to enable us to abbreviate WITH clause for now.
Also, at this time that FORCE option is removed, we could bring back
idea is to put VERBOSE at after object name like CLUSTER. (INDEX,
TABLE, etc.)
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 998340c..2e8db5a 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -21,7 +21,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
-REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ WITH ] [ VERBOSE ]
/synopsis
/refsynopsisdiv
@@ -150,6 +150,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index ac3b785..27364ec 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3133,13 +3134,18 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+bool verbose)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = verbose ? INFO : DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3283,6 +3289,13 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ ereport(elevel,
+ (errmsg(index \%s\ was reindexed.,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3324,7 +3337,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid relid,
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas robertmh...@gmail.com wrote:
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
I thought we agreed on moving this earlier in the command:
http://www.postgresql.org/message-id/18569.1423159...@sss.pgh.pa.us
Oh, I see.
Attached patch is modified syntax as
REINDEX [VERBOSE] { INDEX | ... } name
Thought?
Regards,
---
Sawada Masahiko
000_reindex_verbose_v11.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko sawada.m...@gmail.com wrote:
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
I thought we agreed on moving this earlier in the command:
http://www.postgresql.org/message-id/18569.1423159...@sss.pgh.pa.us
--
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko sawada.m...@gmail.com wrote:
On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas robertmh...@gmail.com wrote:
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
I thought we agreed on moving this earlier in the command:
http://www.postgresql.org/message-id/18569.1423159...@sss.pgh.pa.us
Oh, I see.
Attached patch is modified syntax as
REINDEX [VERBOSE] { INDEX | ... } name
Thought?
I thought what we agreed on was:
REINDEX (flexible options) { INDEX | ... } name
The argument wasn't about whether to use flexible options, but where
in the command to put them.
--
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, May 1, 2015 at 1:38 AM, Robert Haas robertmh...@gmail.com wrote:
On Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas robertmh...@gmail.com wrote:
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
I thought we agreed on moving this earlier in the command:
http://www.postgresql.org/message-id/18569.1423159...@sss.pgh.pa.us
Oh, I see.
Attached patch is modified syntax as
REINDEX [VERBOSE] { INDEX | ... } name
Thought?
I thought what we agreed on was:
REINDEX (flexible options) { INDEX | ... } name
The argument wasn't about whether to use flexible options, but where
in the command to put them.
VACUUM has both syntax: with parentheses and without parentheses.
I think we should have both syntax for REINDEX like VACUUM does
because it would be pain to put parentheses whenever we want to do
REINDEX.
Are the parentheses optional in REINDEX command?
And CLUSTER should have syntax like that in future?
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 30, 2015 at 10:15 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas robertmh...@gmail.com
wrote:
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Attached v10 patch is latest version patch.
The syntax is,
REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
That is, WITH clause is optional.
I thought we agreed on moving this earlier in the command:
http://www.postgresql.org/message-id/18569.1423159...@sss.pgh.pa.us
Oh, I see.
Attached patch is modified syntax as
REINDEX [VERBOSE] { INDEX | ... } name
Thought?
Looks good to me.
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Apr 9, 2015 at 1:14 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Thank you for taking time for this patch! I removed the FORCE option from REINDEX, so you would need to update the patch. Thanks. I will change the patch with this change. This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... Okay, another question is that; WITH must be required whenever the options are specified? Or should it be abbreviatable? In previous discussion, the WITH clause is always required by VERBOSE option. I don't think to enable us to abbreviate WITH clause for now. Also, at this time that FORCE option is removed, we could bring back idea is to put VERBOSE at after object name like CLUSTER. (INDEX, TABLE, etc.) 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Thank you for taking time for this patch! This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Thank you for taking time for this patch! I removed the FORCE option from REINDEX, so you would need to update the patch. This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... Okay, another question is that; WITH must be required whenever the options are specified? Or should it be abbreviatable? 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Apr 7, 2015 at 9:32 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello, I have some trivial comments about the latest patch.
At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko
sawada.m...@gmail.com wrote in
CAD21AoBxPCpPvKQmvJMUh+p=2pfau03gkjq2r2zy47xhsh2...@mail.gmail.com
sawada.mshk On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby
jim.na...@bluetreble.com wrote:
Are the parenthesis necessary? No other WITH option requires them,
other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
The thing is, ()s are actually an odd-duck. Very little supports it,
and
while COPY allows it they're not required. EXPLAIN is a different
story,
because that's not WITH; we're actually using () *instead of* WITH.
So because almost all commands that use WITH doen't even accept (), I
don't
think this should either. It certainly shouldn't require them,
because
unlike EXPLAIN, there's no need to require them.
I understood what your point is.
Attached patch is changed syntax, it does not have parenthesis.
As I looked into the code to find what the syntax would be, I
found some points which would be better to be fixed.
In gram.y the options is a list of cstring but it is not necesary
to be a list because there's only one kind of option now.
If you prefer it to be a list, I have a comment for the way to
make string list in gram.y. You stored bare cstring in the
options list but I think it is not the preferable form. I suppose
the followings are preferable. Corresponding fixes are needed in
ReindexTable, ReindexIndex, ReindexMultipleTables.
$ = list_make1(makeString($1);
$ = lappend($1, list_make1(makeString($3));
In equalfuncs.c, _equalReindexStmt forgets to compare the member
options. _copyReindexStmt also forgets to copy it. The way you
constructed the options list prevents them from doing their jobs
using prepared methods. Comparing and copying the member option
is needed even if it becomes a simple string.
I revised patch, and changed gram.y as I don't use the list.
So this patch adds new syntax,
REINDEX { INDEX | ... } name WITH VERBOSE;
Also documentation is updated.
Please give me feedbacks.
Some notes:
1) There are a trailing space in src/backend/parser/gram.y:
- | REINDEX DATABASE name opt_force
+ | REINDEX reindex_target_multitable name WITH opt_verbose
{
ReindexStmt *n = makeNode(ReindexStmt);
- n-kind = REINDEX_OBJECT_DATABASE;
+ n-kind = $2;
n-name = $3;
n-relation = NULL;
+ n-verbose = $5;
$$ = (Node *)n;
}
;
2) The documentation was updated and is according the behaviour.
3) psql autocomplete is ok.
4) Lack of regression tests. I think you should add some regression like
that:
fabrizio=# \set VERBOSITY terse
fabrizio=# create table reindex_verbose(id integer primary key);
CREATE TABLE
fabrizio=# reindex table reindex_verbose with verbose;
INFO: index reindex_verbose_pkey was reindexed.
REINDEX
5) Code style and organization is ok
6) You should add the new field ReindexStmt-verbose to
src/backend/nodes/copyfuncs.c
Regards,
Thank you for your reviewing.
I modified the patch and attached latest version patch(v7).
Please have a look it.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 0a4c7d4..27be1a4 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable WITH VERBOSE
/synopsis
/refsynopsisdiv
@@ -159,6 +160,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 351dcb2..fc44495 100644
--- a/src/backend/catalog/index.c
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
Thank you for your reviewing.
I modified the patch and attached latest version patch(v7).
Please have a look it.
Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y.
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
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 0a4c7d4..27be1a4 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable WITH VERBOSE
/synopsis
/refsynopsisdiv
@@ -159,6 +160,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 351dcb2..fc44495 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3133,13 +3134,18 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+bool verbose)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = verbose ? INFO : DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3283,6 +3289,13 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ ereport(elevel,
+ (errmsg(index \%s\ was reindexed.,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3324,7 +3337,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid relid, int flags, bool verbose)
{
Relation rel;
Oid toast_relid;
@@ -3415,7 +3428,7 @@ reindex_relation(Oid relid, int flags)
RelationSetIndexList(rel, doneIndexes, InvalidOid);
reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS),
- persistence);
+ persistence, verbose);
CommandCounterIncrement();
@@ -3450,7 +3463,7 @@ reindex_relation(Oid relid, int flags)
* still hold the lock on the master table.
*/
if ((flags REINDEX_REL_PROCESS_TOAST) OidIsValid(toast_relid))
- result |= reindex_relation(toast_relid, flags);
+ result |= reindex_relation(toast_relid, flags, verbose);
return result;
}
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 3febdd5..34ffaba 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1532,7 +1532,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
reindex_flags |= REINDEX_REL_FORCE_INDEXES_PERMANENT;
- reindex_relation(OIDOldHeap, reindex_flags);
+ reindex_relation(OIDOldHeap, reindex_flags, false);
/*
* If the relation being rebuild is pg_class, swap_relation_files()
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 99acd4a..a42a508 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1681,7 +1681,7 @@ ChooseIndexColumnNames(List *indexElems)
* Recreate a specific index.
*/
Oid
-ReindexIndex(RangeVar *indexRelation)
+ReindexIndex(RangeVar *indexRelation, bool verbose)
{
Oid indOid;
Oid heapOid = InvalidOid;
@@ -1706,7 +1706,7 @@ ReindexIndex(RangeVar *indexRelation)
persistence = irel-rd_rel-relpersistence;
index_close(irel, NoLock);
- reindex_index(indOid, false, persistence);
+ reindex_index(indOid, false, persistence, verbose);
return indOid;
}
@@ -1775,7 +1775,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* Recreate
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. Thank you for final reviewing! 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko sawada.m...@gmail.com wrote: On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko sawada.m...@gmail.com wrote: On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko sawada.m...@gmail.com wrote: Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as Ready for Committer? +1 Changed status to Ready for Committer. The patch adds new syntax like REINDEX ... WITH VERBOSE, i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello, I have some trivial comments about the latest patch.
At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko
sawada.m...@gmail.com wrote in CAD21AoBxPCpPvKQmvJMUh+p=
2pfau03gkjq2r2zy47xhsh2...@mail.gmail.com
sawada.mshk On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby
jim.na...@bluetreble.com wrote:
Are the parenthesis necessary? No other WITH option requires
them, other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
The thing is, ()s are actually an odd-duck. Very little supports it,
and
while COPY allows it they're not required. EXPLAIN is a different
story,
because that's not WITH; we're actually using () *instead of* WITH.
So because almost all commands that use WITH doen't even accept (),
I don't
think this should either. It certainly shouldn't require them,
because
unlike EXPLAIN, there's no need to require them.
I understood what your point is.
Attached patch is changed syntax, it does not have parenthesis.
As I looked into the code to find what the syntax would be, I
found some points which would be better to be fixed.
In gram.y the options is a list of cstring but it is not necesary
to be a list because there's only one kind of option now.
If you prefer it to be a list, I have a comment for the way to
make string list in gram.y. You stored bare cstring in the
options list but I think it is not the preferable form. I suppose
the followings are preferable. Corresponding fixes are needed in
ReindexTable, ReindexIndex, ReindexMultipleTables.
$ = list_make1(makeString($1);
$ = lappend($1, list_make1(makeString($3));
In equalfuncs.c, _equalReindexStmt forgets to compare the member
options. _copyReindexStmt also forgets to copy it. The way you
constructed the options list prevents them from doing their jobs
using prepared methods. Comparing and copying the member option
is needed even if it becomes a simple string.
I revised patch, and changed gram.y as I don't use the list.
So this patch adds new syntax,
REINDEX { INDEX | ... } name WITH VERBOSE;
Also documentation is updated.
Please give me feedbacks.
Some notes:
1) There are a trailing space in src/backend/parser/gram.y:
- | REINDEX DATABASE name opt_force
+ | REINDEX reindex_target_multitable name WITH opt_verbose
{
ReindexStmt *n = makeNode(ReindexStmt);
- n-kind = REINDEX_OBJECT_DATABASE;
+ n-kind = $2;
n-name = $3;
n-relation = NULL;
+ n-verbose = $5;
$$ = (Node *)n;
}
;
2) The documentation was updated and is according the behaviour.
3) psql autocomplete is ok.
4) Lack of regression tests. I think you should add some regression like
that:
fabrizio=# \set VERBOSITY terse
fabrizio=# create table reindex_verbose(id integer primary key);
CREATE TABLE
fabrizio=# reindex table reindex_verbose with verbose;
INFO: index reindex_verbose_pkey was reindexed.
REINDEX
5) Code style and organization is ok
6) You should add the new field ReindexStmt-verbose to
src/backend/nodes/copyfuncs.c
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello, I have some trivial comments about the latest patch.
At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko sawada.m...@gmail.com
wrote in CAD21AoBxPCpPvKQmvJMUh+p=2pfau03gkjq2r2zy47xhsh2...@mail.gmail.com
sawada.mshk On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby
jim.na...@bluetreble.com wrote:
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
The thing is, ()s are actually an odd-duck. Very little supports it, and
while COPY allows it they're not required. EXPLAIN is a different story,
because that's not WITH; we're actually using () *instead of* WITH.
So because almost all commands that use WITH doen't even accept (), I don't
think this should either. It certainly shouldn't require them, because
unlike EXPLAIN, there's no need to require them.
I understood what your point is.
Attached patch is changed syntax, it does not have parenthesis.
As I looked into the code to find what the syntax would be, I
found some points which would be better to be fixed.
In gram.y the options is a list of cstring but it is not necesary
to be a list because there's only one kind of option now.
If you prefer it to be a list, I have a comment for the way to
make string list in gram.y. You stored bare cstring in the
options list but I think it is not the preferable form. I suppose
the followings are preferable. Corresponding fixes are needed in
ReindexTable, ReindexIndex, ReindexMultipleTables.
$$ = list_make1(makeString($1);
$$ = lappend($1, list_make1(makeString($3));
In equalfuncs.c, _equalReindexStmt forgets to compare the member
options. _copyReindexStmt also forgets to copy it. The way you
constructed the options list prevents them from doing their jobs
using prepared methods. Comparing and copying the member option
is needed even if it becomes a simple string.
I revised patch, and changed gram.y as I don't use the list.
So this patch adds new syntax,
REINDEX { INDEX | ... } name WITH VERBOSE;
Also documentation is updated.
Please give me feedbacks.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 0a4c7d4..27be1a4 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,7 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable WITH VERBOSE
/synopsis
/refsynopsisdiv
@@ -159,6 +160,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 351dcb2..fc44495 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3133,13 +3134,18 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+bool verbose)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = verbose ? INFO : DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3283,6 +3289,13 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ ereport(elevel,
+ (errmsg(index \%s\ was reindexed.,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3324,7 +3337,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Mar 11, 2015 at 5:36 PM, Jim Nasby jim.na...@bluetreble.com wrote: The thing is, ()s are actually an odd-duck. Very little supports it, and while COPY allows it they're not required. EXPLAIN is a different story, because that's not WITH; we're actually using () *instead of* WITH. Generally, I think the commands that don't have () are the older ones, and those that do have it are the newer ones: EXPLAIN, VERBOSE, the newest of our three COPY syntaxes, CREATE MATERIALIZED VIEW, foreign data wrappers, servers, and foreign tables. The older stuff like CREATE DATABASE and REINDEX that uses ad-hoc syntax instead is a real pain in the neck: every time you want to add an option, you've got to add new parser rules and keywords, which is bad for the overall efficiency of parsing. So I think this argument is exactly backwards: parenthesized options are the newer, better way to do this sort of thing. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 3/13/15 6:48 AM, Robert Haas wrote: On Wed, Mar 11, 2015 at 5:36 PM, Jim Nasby jim.na...@bluetreble.com wrote: The thing is, ()s are actually an odd-duck. Very little supports it, and while COPY allows it they're not required. EXPLAIN is a different story, because that's not WITH; we're actually using () *instead of* WITH. Generally, I think the commands that don't have () are the older ones, and those that do have it are the newer ones: EXPLAIN, VERBOSE, the newest of our three COPY syntaxes, CREATE MATERIALIZED VIEW, foreign data wrappers, servers, and foreign tables. The older stuff like CREATE DATABASE and REINDEX that uses ad-hoc syntax instead is a real pain in the neck: every time you want to add an option, you've got to add new parser rules and keywords, which is bad for the overall efficiency of parsing. So I think this argument is exactly backwards: parenthesized options are the newer, better way to do this sort of thing. Yeah, that doesn't sound like a good tradeoff compared to making people type some extra ()s. :( We should at least support ()s on the other commands though, so that we're consistent. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, Mar 13, 2015 at 8:57 AM, Jim Nasby jim.na...@bluetreble.com wrote: Yeah, that doesn't sound like a good tradeoff compared to making people type some extra ()s. :( We should at least support ()s on the other commands though, so that we're consistent. I think we've been moving slowly in that direction, but it's not this patch's job to accelerate that transition. -- 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Mar 10, 2015 at 5:05 PM, Jim Nasby jim.na...@bluetreble.com wrote:
On 3/9/15 9:43 PM, Sawada Masahiko wrote:
On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 3/2/15 10:58 AM, Sawada Masahiko wrote:
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 2/24/15 8:28 AM, Sawada Masahiko wrote:
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the*third additional* option
sytle, but it is the different discussion from this)
Well, almost everything does a trailing WITH. We need to either stick
with
that for consistency, or add leading () as an option to those WITH
commands.
Does anyone know why those are WITH? Is it ANSI?
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH
DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the
most
consistent with everything else. Is there a problem with doing that? I
know
getting syntax is one of the hard parts of new features, but it seems
like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
We have discussed about this option including FORCE option, but I
think there are not user who want to use both FORCE and VERBOSE option
at same time.
I find that very hard to believe... I would expect a primary use case for
VERBOSE to be I ran REINDEX, but it doesn't seem to have done
anything...
what's going on? and that's exactly when you might want to use FORCE.
In currently code, nothing happens even if FORCE option is specified.
This option completely exist for backward compatibility.
But this patch add new syntax including FORCE option for now.
I forgot that. There's no reason to support it with the new stuff then.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
Attached patch is latest version patch changed syntax a little.
This patch adds following syntax for now.
REINDEX { INDEX | ... } name WITH (VERBOSE);
But we are under the discussion regarding parenthesis, so there is
possibility of change.
Regards,
---
Sawada Masahiko
000_reindex_verbose_v4.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby jim.na...@bluetreble.com wrote:
On 3/11/15 6:33 AM, Sawada Masahiko wrote:
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry
WITH
DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be
the
most
consistent with everything else. Is there a problem with doing that?
I
know
getting syntax is one of the hard parts of new features, but it
seems
like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
The thing is, ()s are actually an odd-duck. Very little supports it, and
while COPY allows it they're not required. EXPLAIN is a different story,
because that's not WITH; we're actually using () *instead of* WITH.
So because almost all commands that use WITH doen't even accept (), I don't
think this should either. It certainly shouldn't require them, because
unlike EXPLAIN, there's no need to require them.
I understood what your point is.
Attached patch is changed syntax, it does not have parenthesis.
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 0a4c7d4..3cea35f 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,11 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable WITH replaceable class=PARAMETERoptions/replaceable [, ...]
+
+phrasewhere replaceable class=PARAMETERoption/replaceable can be one of:/phrase
+
+VERBOSE
/synopsis
/refsynopsisdiv
@@ -159,6 +164,15 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f85ed93..786f173 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3130,13 +3131,18 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+bool verbose)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = verbose ? INFO : DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3280,6 +3286,13 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
heap_close(pg_index, RowExclusiveLock);
}
+ /* Log what we did */
+ ereport(elevel,
+ (errmsg(index \%s\ was reindexed.,
+ get_rel_name(indexId)),
+ errdetail(%s.,
+ pg_rusage_show(ru0;
+
/* Close rels, but keep locks */
index_close(iRel, NoLock);
heap_close(heapRelation, NoLock);
@@ -3321,7 +3334,7 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
* index rebuild.
*/
bool
-reindex_relation(Oid relid, int flags)
+reindex_relation(Oid relid, int flags, bool verbose)
{
Relation rel;
Oid toast_relid;
@@ -3412,7 +3425,7 @@ reindex_relation(Oid relid, int flags)
RelationSetIndexList(rel, doneIndexes, InvalidOid);
reindex_index(indexOid, !(flags REINDEX_REL_CHECK_CONSTRAINTS),
- persistence);
+ persistence, verbose);
CommandCounterIncrement();
@@
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 3/11/15 6:33 AM, Sawada Masahiko wrote:
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH
DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the
most
consistent with everything else. Is there a problem with doing that? I
know
getting syntax is one of the hard parts of new features, but it seems
like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
The thing is, ()s are actually an odd-duck. Very little supports it, and
while COPY allows it they're not required. EXPLAIN is a different story,
because that's not WITH; we're actually using () *instead of* WITH.
So because almost all commands that use WITH doen't even accept (), I
don't think this should either. It certainly shouldn't require them,
because unlike EXPLAIN, there's no need to require them.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Mar 10, 2015 at 5:05 PM, Jim Nasby jim.na...@bluetreble.com wrote:
On 3/9/15 9:43 PM, Sawada Masahiko wrote:
On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 3/2/15 10:58 AM, Sawada Masahiko wrote:
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 2/24/15 8:28 AM, Sawada Masahiko wrote:
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the*third additional* option
sytle, but it is the different discussion from this)
Well, almost everything does a trailing WITH. We need to either stick
with
that for consistency, or add leading () as an option to those WITH
commands.
Does anyone know why those are WITH? Is it ANSI?
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH
DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the
most
consistent with everything else. Is there a problem with doing that? I
know
getting syntax is one of the hard parts of new features, but it seems
like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
I was imagining EXPLAIN syntax.
Is there some possibility of supporting multiple options for REINDEX
command in future?
If there is, syntax will be as follows, REINDEX { INDEX | ... } name
WITH VERBOSE, XXX, XXX;
I thought style with parenthesis is better than above style.
We have discussed about this option including FORCE option, but I
think there are not user who want to use both FORCE and VERBOSE option
at same time.
I find that very hard to believe... I would expect a primary use case for
VERBOSE to be I ran REINDEX, but it doesn't seem to have done
anything...
what's going on? and that's exactly when you might want to use FORCE.
In currently code, nothing happens even if FORCE option is specified.
This option completely exist for backward compatibility.
But this patch add new syntax including FORCE option for now.
I forgot that. There's no reason to support it with the new stuff then.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 3/9/15 9:43 PM, Sawada Masahiko wrote:
On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby jim.na...@bluetreble.com wrote:
On 3/2/15 10:58 AM, Sawada Masahiko wrote:
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 2/24/15 8:28 AM, Sawada Masahiko wrote:
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the*third additional* option
sytle, but it is the different discussion from this)
Well, almost everything does a trailing WITH. We need to either stick
with
that for consistency, or add leading () as an option to those WITH
commands.
Does anyone know why those are WITH? Is it ANSI?
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most
consistent with everything else. Is there a problem with doing that? I know
getting syntax is one of the hard parts of new features, but it seems like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
Are the parenthesis necessary? No other WITH option requires them, other
than create table/matview (COPY doesn't actually require them).
We have discussed about this option including FORCE option, but I
think there are not user who want to use both FORCE and VERBOSE option
at same time.
I find that very hard to believe... I would expect a primary use case for
VERBOSE to be I ran REINDEX, but it doesn't seem to have done anything...
what's going on? and that's exactly when you might want to use FORCE.
In currently code, nothing happens even if FORCE option is specified.
This option completely exist for backward compatibility.
But this patch add new syntax including FORCE option for now.
I forgot that. There's no reason to support it with the new stuff then.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby jim.na...@bluetreble.com wrote:
On 3/2/15 10:58 AM, Sawada Masahiko wrote:
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com
wrote:
On 2/24/15 8:28 AM, Sawada Masahiko wrote:
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the*third additional* option
sytle, but it is the different discussion from this)
Well, almost everything does a trailing WITH. We need to either stick
with
that for consistency, or add leading () as an option to those WITH
commands.
Does anyone know why those are WITH? Is it ANSI?
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most
consistent with everything else. Is there a problem with doing that? I know
getting syntax is one of the hard parts of new features, but it seems like
we reached consensus here...
Attached is latest version patch based on Tom's idea as follows.
REINDEX { INDEX | ... } name WITH ( options [, ...] )
We have discussed about this option including FORCE option, but I
think there are not user who want to use both FORCE and VERBOSE option
at same time.
I find that very hard to believe... I would expect a primary use case for
VERBOSE to be I ran REINDEX, but it doesn't seem to have done anything...
what's going on? and that's exactly when you might want to use FORCE.
In currently code, nothing happens even if FORCE option is specified.
This option completely exist for backward compatibility.
But this patch add new syntax including FORCE option for now.
Todo
- tab completion
- reindexdb command
Regards,
---
Sawada Masahiko
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index 0a4c7d4..a4109aa 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -22,6 +22,12 @@ PostgreSQL documentation
refsynopsisdiv
synopsis
REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable [ FORCE ]
+REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAMETERname/replaceable WITH ( replaceable class=PARAMETERoptions/replaceable [, ...] )
+
+phrasewhere replaceable class=PARAMETERoption/replaceable can be one of:/phrase
+
+FORCE [ replaceable class=PARAMETERboolean/replaceable ]
+VERBOSE [ replaceable class=PARAMETERboolean/replaceable ]
/synopsis
/refsynopsisdiv
@@ -159,6 +165,29 @@ REINDEX { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } replaceable class=PARAM
/para
/listitem
/varlistentry
+
+ varlistentry
+termliteralVERBOSE/literal/term
+listitem
+ para
+ Prints a progress report as each index is reindexed.
+ /para
+/listitem
+ /varlistentry
+
+ varlistentry
+termreplaceable class=parameterboolean/replaceable/term
+listitem
+ para
+ Specifies whether the selected option should be turned on or off.
+ You can write literalTRUE/literal, literalON/, or
+ literal1/literal to enable the option, and literalFALSE/literal,
+ literalOFF/, or literal0/literal to disable it. The
+ replaceable class=parameterboolean/replaceable value can also
+ be omitted, in which case literalTRUE/literal is assumed.
+ /para
+/listitem
+ /varlistentry
/variablelist
/refsect1
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f85ed93..786f173 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -63,6 +63,7 @@
#include utils/inval.h
#include utils/lsyscache.h
#include utils/memutils.h
+#include utils/pg_rusage.h
#include utils/syscache.h
#include utils/tuplesort.h
#include utils/snapmgr.h
@@ -3130,13 +3131,18 @@ IndexGetRelation(Oid indexId, bool missing_ok)
* reindex_index - This routine is used to recreate a single index
*/
void
-reindex_index(Oid indexId, bool skip_constraint_checks, char persistence)
+reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
+bool verbose)
{
Relation iRel,
heapRelation;
Oid heapId;
IndexInfo *indexInfo;
volatile bool skipped_constraint = false;
+ int elevel = verbose ? INFO : DEBUG2;
+ PGRUsage ru0;
+
+ pg_rusage_init(ru0);
/*
* Open and lock the parent heap relation. ShareLock is sufficient since
@@ -3280,6 +3286,13 @@
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 3/2/15 10:58 AM, Sawada Masahiko wrote: On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com wrote: On 2/24/15 8:28 AM, Sawada Masahiko wrote: According to the above discussion, VACUUM and REINDEX should have trailing options. Tom seems (to me) suggesting that SQL-style (bare word preceded by WITH) options and Jim suggesting '()' style options? (Anyway VACUUM gets the*third additional* option sytle, but it is the different discussion from this) Well, almost everything does a trailing WITH. We need to either stick with that for consistency, or add leading () as an option to those WITH commands. Does anyone know why those are WITH? Is it ANSI? As a refresher, current commands are: VACUUM (ANALYZE, VERBOSE) table1 (col1); REINDEX INDEX index1 FORCE; COPY table1 FROM 'file.txt' WITH (FORMAT csv); CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; CREATE ROLE role WITH LOGIN; GRANT WITH GRANT OPTION; CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; ALTER DATABASE db1 WITH CONNECTION LIMIT 50; DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most consistent with everything else. Is there a problem with doing that? I know getting syntax is one of the hard parts of new features, but it seems like we reached consensus here... We have discussed about this option including FORCE option, but I think there are not user who want to use both FORCE and VERBOSE option at same time. I find that very hard to believe... I would expect a primary use case for VERBOSE to be I ran REINDEX, but it doesn't seem to have done anything... what's going on? and that's exactly when you might want to use FORCE. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby jim.na...@bluetreble.com wrote:
On 2/24/15 8:28 AM, Sawada Masahiko wrote:
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the*third additional* option
sytle, but it is the different discussion from this)
Well, almost everything does a trailing WITH. We need to either stick with
that for consistency, or add leading () as an option to those WITH commands.
Does anyone know why those are WITH? Is it ANSI?
As a refresher, current commands are:
VACUUM (ANALYZE, VERBOSE) table1 (col1);
REINDEX INDEX index1 FORCE;
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA;
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
CREATE ROLE role WITH LOGIN;
GRANT WITH GRANT OPTION;
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
We have discussed about this option including FORCE option, but I
think there are not user who want to use both FORCE and VERBOSE option
at same time.
Can we think and add new syntax without FORCE option while leaving
current Reindex statement syntax?
As prototype, I attached new version patch has the following syntax.
REINDEX { INDEX | TABLE | ... } relname [ FORCE | VERBOSE];
Regards,
---
Sawada Masahiko
000_reindex_verbose_v2.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Fri, Feb 20, 2015 at 12:24 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
Hello,
I showed an extreme number of examples to include *almost of all*
variations of existing syntax of option specification. And showed
what if all variations could be used for all commands. It was
almost a mess. Sorry for the confusion.
I think the issues at our hands are,
- Options location: at-the-end or right-after-the-keyword?
- FORCE options to be removed?
- Decide whether to allow bare word option if the options are to
be located right after the keyword.
Optinions or thoughts?
Rethinking from here.
At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby jim.na...@bluetreble.com
wrote in 54dbbcc9.1020...@bluetreble.com
On 2/5/15 12:01 PM, Tom Lane wrote:
...
Meh. Options-at-the-end seems by far the most sensible style to me.
The options-right-after-the-keyword style is a mess, both logically
and from a parsing standpoint, and the only reason we have it at all
is historical artifact (ask Bruce about the origins of VACUUM ANALYZE
over a beer sometime).
...
I know you were worried about accepting options anywhere because it
leads to reserved words, but perhaps we could support it just for
EXPLAIN and VACUUM, and then switch to trailing options if people
think that would be better.
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the *third additional* option
sytle, but it is the different discussion from this)
VACUUM [tname [(cname, ...)]] [({FULL [bool]|FREEZE | [bool]|...})]
=# VACUUM t1 (FULL, FREEZE);
VACUUM [tname [(cname, ...)]] [WITH [FULL [bool]]|[FREEZE | [bool]|...]
=# VACUUM t1 WITH FULL;
IMHO, we are so accustomed to call by the names VACUUM FULL or
VACUUM FREEZE that the both of them look a bit uneasy.
If the new syntax above is added, REINDEX should have *only* the
trailing style.
REINDEX [{INDEX|TABLE|...}] name [(VERBOSE [bool]|...)]
=# REINDEX TABLE t1 (VERBOSE);
REINDEX [{INDEX|TABLE|...}] name [WITH {VERBOSE [bool]|...}]
=# REINDEX INDEX i_t1_pkey WITH VERBOSE;
Also, both of them seems to be unintuitive..
EXPLAIN.. it seems to be preferred to be as it is..
As the result, it seems the best way to go on the current syntax
for all of those commands.
Optinions?
At Wed, 18 Feb 2015 23:58:15 +0900, Sawada Masahiko sawada.m...@gmail.com
wrote in cad21aobkjndqaq2z-wbscmdhox257bjj6suythwwfs2il8y...@mail.gmail.com
From consistency perspective, I tend to agree with Robert to put
option at immediately after command name as follows.
REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name;
I don't object against it if you prefer it. The remaining issue
is the choice between options-at-the-end or this
options-right-after-the-keyword mentioned above. I prefer the
more messy(:-) one..
Btw how long will the FORCE command available?
The options is obsolete since 7.4. I think it should have been
fade away long since and it's the time to remove it. But once the
ancient option removed, the above syntax looks a bit uneasy and
the more messy syntax looks natural.
REINDEX [VERBOSE] {INDEX | ...} name;
That do you think about this?
Thank you for summarizing them.
I said right-after-the-keyword is looks good to me.
But it's will be possible only if FORCE command is removed.
REINDEX command has FORCE option at the end, so REINDEX probably
should have options at the end.
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/24/15 8:28 AM, Sawada Masahiko wrote: According to the above discussion, VACUUM and REINDEX should have trailing options. Tom seems (to me) suggesting that SQL-style (bare word preceded by WITH) options and Jim suggesting '()' style options? (Anyway VACUUM gets the*third additional* option sytle, but it is the different discussion from this) Well, almost everything does a trailing WITH. We need to either stick with that for consistency, or add leading () as an option to those WITH commands. Does anyone know why those are WITH? Is it ANSI? As a refresher, current commands are: VACUUM (ANALYZE, VERBOSE) table1 (col1); REINDEX INDEX index1 FORCE; COPY table1 FROM 'file.txt' WITH (FORMAT csv); CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; CREATE ROLE role WITH LOGIN; GRANT WITH GRANT OPTION; CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; ALTER DATABASE db1 WITH CONNECTION LIMIT 50; DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Hello,
I showed an extreme number of examples to include *almost of all*
variations of existing syntax of option specification. And showed
what if all variations could be used for all commands. It was
almost a mess. Sorry for the confusion.
I think the issues at our hands are,
- Options location: at-the-end or right-after-the-keyword?
- FORCE options to be removed?
- Decide whether to allow bare word option if the options are to
be located right after the keyword.
Optinions or thoughts?
Rethinking from here.
At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby jim.na...@bluetreble.com wrote
in 54dbbcc9.1020...@bluetreble.com
On 2/5/15 12:01 PM, Tom Lane wrote:
...
Meh. Options-at-the-end seems by far the most sensible style to me.
The options-right-after-the-keyword style is a mess, both logically
and from a parsing standpoint, and the only reason we have it at all
is historical artifact (ask Bruce about the origins of VACUUM ANALYZE
over a beer sometime).
...
I know you were worried about accepting options anywhere because it
leads to reserved words, but perhaps we could support it just for
EXPLAIN and VACUUM, and then switch to trailing options if people
think that would be better.
According to the above discussion, VACUUM and REINDEX should have
trailing options. Tom seems (to me) suggesting that SQL-style
(bare word preceded by WITH) options and Jim suggesting '()'
style options? (Anyway VACUUM gets the *third additional* option
sytle, but it is the different discussion from this)
VACUUM [tname [(cname, ...)]] [({FULL [bool]|FREEZE | [bool]|...})]
=# VACUUM t1 (FULL, FREEZE);
VACUUM [tname [(cname, ...)]] [WITH [FULL [bool]]|[FREEZE | [bool]|...]
=# VACUUM t1 WITH FULL;
IMHO, we are so accustomed to call by the names VACUUM FULL or
VACUUM FREEZE that the both of them look a bit uneasy.
If the new syntax above is added, REINDEX should have *only* the
trailing style.
REINDEX [{INDEX|TABLE|...}] name [(VERBOSE [bool]|...)]
=# REINDEX TABLE t1 (VERBOSE);
REINDEX [{INDEX|TABLE|...}] name [WITH {VERBOSE [bool]|...}]
=# REINDEX INDEX i_t1_pkey WITH VERBOSE;
Also, both of them seems to be unintuitive..
EXPLAIN.. it seems to be preferred to be as it is..
As the result, it seems the best way to go on the current syntax
for all of those commands.
Optinions?
At Wed, 18 Feb 2015 23:58:15 +0900, Sawada Masahiko sawada.m...@gmail.com
wrote in cad21aobkjndqaq2z-wbscmdhox257bjj6suythwwfs2il8y...@mail.gmail.com
From consistency perspective, I tend to agree with Robert to put
option at immediately after command name as follows.
REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name;
I don't object against it if you prefer it. The remaining issue
is the choice between options-at-the-end or this
options-right-after-the-keyword mentioned above. I prefer the
more messy(:-) one..
Btw how long will the FORCE command available?
The options is obsolete since 7.4. I think it should have been
fade away long since and it's the time to remove it. But once the
ancient option removed, the above syntax looks a bit uneasy and
the more messy syntax looks natural.
REINDEX [VERBOSE] {INDEX | ...} name;
That do you think about this?
regards,
At Tue, 17 Feb 2015 12:00:13 -0600, Jim Nasby jim.na...@bluetreble.com wrote
in 54e381ad.1090...@bluetreble.com
VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)]
| VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)]
| VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE
| [bool]|...})]
REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)]
EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})]
statement
For concrete examples, the lines prefixed by asterisk are in new
syntax.
If I could choose only one for explain, I would find it easier to be
up front. That way you do the explain part on one line and just paste
the query after that.
..
VACUUM FULL table1;
VACUUM ANALYZE table1 (col1);
VACUUM (ANALYZE, VERBOSE) table1 (col1);
*VACUUM table1 WITH (FREEZE on)
*VACUUM table1 (cola) WITH (ANALYZE)
*VACUUM table1 WITH (ANALYZE)
*VACUUM table1 (FREEZE on)
The fifth example looks quite odd.
I don't think we need to allow both () and WITH... I'd say one or the
other, preferably ().
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)]
| VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)]
| VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})]
REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)]
EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] statement
I don't think (OptionName [bool]) style like (VERBOSE on, FORCE
on) is needed for REINDEX command.
EXPLAIN command has such option style because it has the FORMAT option
can have value excepting ON/TRUE or OFF/FALSE.(e.g., TEXT, XML)
But the value of REINDEX command option can have only ON or OFF.
I think the option name is good enough.
Next, regarding of the location of such option, the several
maintenance command like CLUSTER, VACUUM has option at immediately
after command name.
From consistency perspective, I tend to agree with Robert to put
option at immediately after command name as follows.
REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name;
Btw how long will the FORCE command available?
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/16/15 9:43 PM, Kyotaro HORIGUCHI wrote:
Hello, I had a look on gram.y and found other syntaxes using WITH
option clause.
At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasbyjim.na...@bluetreble.com wrote
in54dbbcc9.1020...@bluetreble.com
I suspect at least some of this stems from how command line programs
tend to process options before arguments. I tend to agree with you
Tom, but I think what's more important is that we're consistent. COPY
is already a bit of an oddball because it uses WITH, but both EXPLAIN
and VACUUM use parenthesis immediately after the first
verb. Introducing a parenthesis version that goes at the end instead
of the beginning is just going to make this worse.
If we're going to take a stand on this, we need to do it NOW, before
we have even more commands that use ().
I know you were worried about accepting options anywhere because it
leads to reserved words, but perhaps we could support it just for
EXPLAIN and VACUUM, and then switch to trailing options if people
think that would be better.
I agree with the direction, but I see two issues here; how many
syntax variants we are allowed to have for one command at a time,
and how far we should/may extend the unified options syntax on
other commands.
Let me put the issues aside for now, VACUUM can have trailing
options naturally but it seems to change, but, IMHO, EXPLAIN
should have the target statement at the tail end. Are you
thinking of syntaxes like following?
VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)]
| VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)]
| VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})]
REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)]
EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] statement
For concrete examples, the lines prefixed by asterisk are in new
syntax.
If I could choose only one for explain, I would find it easier to be up
front. That way you do the explain part on one line and just paste the
query after that.
VACUUM FULL table1;
VACUUM ANALYZE table1 (col1);
VACUUM (ANALYZE, VERBOSE) table1 (col1);
*VACUUM table1 WITH (FREEZE on)
*VACUUM table1 (cola) WITH (ANALYZE)
*VACUUM table1 WITH (ANALYZE)
*VACUUM table1 (FREEZE on)
The fifth example looks quite odd.
I don't think we need to allow both () and WITH... I'd say one or the
other, preferably ().
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Hello, I had a look on gram.y and found other syntaxes using WITH
option clause.
At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby jim.na...@bluetreble.com wrote
in 54dbbcc9.1020...@bluetreble.com
I suspect at least some of this stems from how command line programs
tend to process options before arguments. I tend to agree with you
Tom, but I think what's more important is that we're consistent. COPY
is already a bit of an oddball because it uses WITH, but both EXPLAIN
and VACUUM use parenthesis immediately after the first
verb. Introducing a parenthesis version that goes at the end instead
of the beginning is just going to make this worse.
If we're going to take a stand on this, we need to do it NOW, before
we have even more commands that use ().
I know you were worried about accepting options anywhere because it
leads to reserved words, but perhaps we could support it just for
EXPLAIN and VACUUM, and then switch to trailing options if people
think that would be better.
I agree with the direction, but I see two issues here; how many
syntax variants we are allowed to have for one command at a time,
and how far we should/may extend the unified options syntax on
other commands.
Let me put the issues aside for now, VACUUM can have trailing
options naturally but it seems to change, but, IMHO, EXPLAIN
should have the target statement at the tail end. Are you
thinking of syntaxes like following?
VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)]
| VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)]
| VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})]
REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)]
EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] statement
For concrete examples, the lines prefixed by asterisk are in new
syntax.
VACUUM FULL table1;
VACUUM ANALYZE table1 (col1);
VACUUM (ANALYZE, VERBOSE) table1 (col1);
*VACUUM table1 WITH (FREEZE on)
*VACUUM table1 (cola) WITH (ANALYZE)
*VACUUM table1 WITH (ANALYZE)
*VACUUM table1 (FREEZE on)
The fifth example looks quite odd.
REINDEX INDEX index1 FORCE;
*REINDEX TABLE table1 WITH (VERBOSE on);
*REINDEX TABLE table1 (VERBOSE on, FORCE on);
EXPLAIN (ANALYZE) SELECT 1;
*EXPLAIN WITH (ANALYZE) SELECT 1;
The WITH looks a bit uneasy..
COPY table1 FROM 'file.txt' WITH (FORMAT csv);
Returning to the second issue, the following statements have
option list (or single option) preceded (or not preceded) by the
word WITH. The prefixing dollar sign indicates that the syntax is
of SQL standard according to the PostgreSQL
Documentation. Asterisk indicates that the line shows the syntax
if the new policy is applied. Other few statements like DECLARE
looks using WITH as a part of an idiom.
CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA;
This is similar to EXPLAIN in the sense that a query follows
it, but this syntax can have the second WITH follows by DATA.
CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over;
*CREATE EXTENSION ext1 WITH (SCHEMA s1, VERSION v1, FROM over);
This seems to fit the unification.
CREATE ROLE role WITH LOGIN;
CREATE ROLE role SUPERUSER, LOGIN;
$CREATE ROLE role WITH ADMIN admin;
*CREATE ROLE role WITH (SUPERUSER, LOGIN);
*CREATE ROLE role (SUPERUSER, LOGIN);
This seems meaninglessly too complecated.
GRANT WITH GRANT OPTION;
*GRANT WITH (GRANT on);
Mmm. Seems no reasoning...
CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION;
*CREATE VIEW v1 AS qry WITH (CASCADED_CHECK);
Wired syntax?
ALTER DATABASE db1 WITH CONNECTION LIMIT 50;
*ALTER DATABASE db1 WITH (CONNECTION_LIMIT 50);
Hardly looks reasonable..
$DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD;
This cannot have another style.
Mmm... I'm at a loss what is desirable..
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/5/15 12:01 PM, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: We've got a mix of styles for extensible options right now: That we do. So COPY puts the options at the very end, but EXPLAIN and VACUUM put them right after the command name. I prefer the latter style and would vote to adopt it here. Meh. Options-at-the-end seems by far the most sensible style to me. The options-right-after-the-keyword style is a mess, both logically and from a parsing standpoint, and the only reason we have it at all is historical artifact (ask Bruce about the origins of VACUUM ANALYZE over a beer sometime). I suspect at least some of this stems from how command line programs tend to process options before arguments. I tend to agree with you Tom, but I think what's more important is that we're consistent. COPY is already a bit of an oddball because it uses WITH, but both EXPLAIN and VACUUM use parenthesis immediately after the first verb. Introducing a parenthesis version that goes at the end instead of the beginning is just going to make this worse. If we're going to take a stand on this, we need to do it NOW, before we have even more commands that use (). I know you were worried about accepting options anywhere because it leads to reserved words, but perhaps we could support it just for EXPLAIN and VACUUM, and then switch to trailing options if people think that would be better. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Robert Haas robertmh...@gmail.com writes: We've got a mix of styles for extensible options right now: That we do. So COPY puts the options at the very end, but EXPLAIN and VACUUM put them right after the command name. I prefer the latter style and would vote to adopt it here. Meh. Options-at-the-end seems by far the most sensible style to me. The options-right-after-the-keyword style is a mess, both logically and from a parsing standpoint, and the only reason we have it at all is historical artifact (ask Bruce about the origins of VACUUM ANALYZE over a beer sometime). Still, I can't help noticing that I'm being outvoted. I'll shut up now. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Tom Lane-2 wrote
Kyotaro HORIGUCHI lt;
horiguchi.kyotaro@.co
gt; writes:
The phrase {INDEX | TABLE |..} name seems to me indivisible as
target specification. IMHO, the options for VACUUM and so is
placed *just after* command name, not *before* the target.
If this is right, the syntax would be like this.
REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name
What do you think about this?
I think this is wrong and ugly. INDEX etc are part of the command name.
I can argue either position...
I lean toward those not being part of the command name because:
The documentation lists only REINDEX as a command while DROP gets a
separate entry for each type of object it is able to drop; mainly because
the behavior of each command could/does differ based upon the target while
REINDEX will still simply cause indexes to be rebuilt and the modifier's
purposes is to aid in the selection of target indexes.
REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | DATABASE | SYSTEM } name
That said, the entire notes section is written like the writer also believed
that REINDEX INDEX is a command in its own right...
VACUUM is a good comparison command and, besides, putting VERBOSE after the
entire thing just doesn't seem right - though that is the only other option
that would work for me. When you read other commands with pre and post
options the wording usually flows reasonably well (IF EXISTS being, for me,
an exception - it reads better after the name, not before, but I
digress...). REINDEX ( VERBOSE ) /target/ reads well to me; I'm already
sold that TABLE name and INDEX name are target specifiers.
David J.
--
View this message in context:
http://postgresql.nabble.com/Proposal-REINDEX-xxx-VERBOSE-tp5836377p5836782.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp writes:
The phrase {INDEX | TABLE |..} name seems to me indivisible as
target specification. IMHO, the options for VACUUM and so is
placed *just after* command name, not *before* the target.
If this is right, the syntax would be like this.
REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name
What do you think about this?
I think this is wrong and ugly. INDEX etc are part of the command name.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Hello,
As per discussion, it seems to good with
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
or
REINDEX { INDEX | TABLE | etc } [ (option [, optoin ...] ) ] name
i.g., the options of reindex(VERBOSE and FORCE) are put at before or
after object name.
Because other maintenance command put option at before object name, I
think the latter is better.
The phrase {INDEX | TABLE |..} name seems to me indivisible as
target specification. IMHO, the options for VACUUM and so is
placed *just after* command name, not *before* the target.
If this is right, the syntax would be like this.
REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name
What do you think about this?
regares,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Wed, Feb 4, 2015 at 2:44 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Jim Nasby jim.na...@bluetreble.com writes:
On 2/3/15 5:08 PM, Tom Lane wrote:
Jim Nasby jim.na...@bluetreble.com writes:
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or
REINDEX {INDEX | ...} (options).
Well, I really really don't like the first of those. IMO the command name
is REINDEX INDEX etc, so sticking something in the middle of that is
bogus.
Actually, is there a reason we can't just accept all 3? Forcing people
to remember exact ordering of options has always struck me as silly.
And that's an even worse idea. Useless flexibility in syntax tends to
lead to unfortunate consequences like having to reserve keywords.
As per discussion, it seems to good with
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
or
REINDEX { INDEX | TABLE | etc } [ (option [, optoin ...] ) ] name
i.g., the options of reindex(VERBOSE and FORCE) are put at before or
after object name.
Because other maintenance command put option at before object name, I
think the latter is better.
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/3/15 5:08 PM, Tom Lane wrote:
Jim Nasby jim.na...@bluetreble.com writes:
On 2/3/15 9:20 AM, Tom Lane wrote:
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or
REINDEX {INDEX | ...} (options).
Well, I really really don't like the first of those. IMO the command name
is REINDEX INDEX etc, so sticking something in the middle of that is
bogus.
Actually, is there a reason we can't just accept all 3? Forcing people
to remember exact ordering of options has always struck me as silly.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Jim Nasby jim.na...@bluetreble.com writes:
On 2/3/15 5:08 PM, Tom Lane wrote:
Jim Nasby jim.na...@bluetreble.com writes:
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or
REINDEX {INDEX | ...} (options).
Well, I really really don't like the first of those. IMO the command name
is REINDEX INDEX etc, so sticking something in the middle of that is
bogus.
Actually, is there a reason we can't just accept all 3? Forcing people
to remember exact ordering of options has always struck me as silly.
And that's an even worse idea. Useless flexibility in syntax tends to
lead to unfortunate consequences like having to reserve keywords.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Feb 3, 2015 at 9:09 AM, Sawada Masahiko sawada.m...@gmail.com
wrote:
On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Sawada Masahiko sawada.m...@gmail.com writes:
On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
Now, I think that it may
be better to provide the keyword VERBOSE before the type of object
reindexed as REINDEX [ VERBOSE ] object.
Actually, my first WIP version of patch added VERBOSE word at before
type of object.
I'm feeling difficult about that the position of VERBOSE word in
REINDEX statement.
The way that FORCE was added to REINDEX was poorly thought out; let's
not
double down on that with another option added without any consideration
for future expansion. I'd be happier if we adopted something similar to
the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in
parentheses.
I understood.
I'm imagining new REINDEX syntax are followings.
- REINDEX (INDEX, VERBOSE) hoge_idx;
- REINDEX (TABLE) hoge_table;
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Thought?
I don't think the keyworks INDEX, TABLE, SCHEMA, SYSTEM and DATABASE are
options... they are part of the command IMHO.
Maybe something like:
REINDEX { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } [( [ FORCE ],
[VERBOSE] ) ] name;
And maintain the old syntax for compatibility of course.
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Sawada Masahiko sawada.m...@gmail.com writes:
On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane t...@sss.pgh.pa.us wrote:
The way that FORCE was added to REINDEX was poorly thought out; let's not
double down on that with another option added without any consideration
for future expansion. I'd be happier if we adopted something similar to
the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in
parentheses.
I understood.
I'm imagining new REINDEX syntax are followings.
- REINDEX (INDEX, VERBOSE) hoge_idx;
- REINDEX (TABLE) hoge_table;
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
option := FORCE | VERBOSE
We'd still keep the historical syntax where you can write FORCE outside
parens, but it'd be deprecated.
Where to insert the parenthesized option list is a judgment call,
but I'd lean to keeping it at the end where FORCE used to be.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2015-02-03 10:20:03 -0500, Tom Lane wrote:
Sawada Masahiko sawada.m...@gmail.com writes:
On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane t...@sss.pgh.pa.us wrote:
The way that FORCE was added to REINDEX was poorly thought out; let's not
double down on that with another option added without any consideration
for future expansion. I'd be happier if we adopted something similar to
the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in
parentheses.
I understood.
I'm imagining new REINDEX syntax are followings.
- REINDEX (INDEX, VERBOSE) hoge_idx;
- REINDEX (TABLE) hoge_table;
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
option := FORCE | VERBOSE
We'd still keep the historical syntax where you can write FORCE outside
parens, but it'd be deprecated.
Why would we allow force inside the parens, given it's a backward compat
only thing afaik? Don't get me wrong, I'm not at all against a
extensible syntax, I just don't see a point in further cargo culting
FORCE.
Greetings,
Andres Freund
--
Andres Freund 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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/3/15 9:20 AM, Tom Lane wrote:
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or
REINDEX {INDEX | ...} (options).
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Andres Freund and...@2ndquadrant.com writes:
On 2015-02-03 10:20:03 -0500, Tom Lane wrote:
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
option := FORCE | VERBOSE
We'd still keep the historical syntax where you can write FORCE outside
parens, but it'd be deprecated.
Why would we allow force inside the parens, given it's a backward compat
only thing afaik? Don't get me wrong, I'm not at all against a
extensible syntax, I just don't see a point in further cargo culting
FORCE.
Ah, I'd forgotten that that option was now a no-op. Yeah, there's no
reason to support it in the new syntax.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Feb 3, 2015 at 8:26 PM, Jim Nasby jim.na...@bluetreble.com wrote:
On 2/3/15 9:20 AM, Tom Lane wrote:
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or REINDEX
{INDEX | ...} (options).
Makes sense... +1
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Jim Nasby jim.na...@bluetreble.com writes:
On 2/3/15 9:20 AM, Tom Lane wrote:
Well, the object type is not an optional part of the command. It's
*necessary*. I was thinking more like
REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]
VACUUM puts the options before the table name, so ISTM it'd be best to
keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or
REINDEX {INDEX | ...} (options).
Well, I really really don't like the first of those. IMO the command name
is REINDEX INDEX etc, so sticking something in the middle of that is
bogus.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Sawada Masahiko sawada.m...@gmail.com writes:
On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
Now, I think that it may
be better to provide the keyword VERBOSE before the type of object
reindexed as REINDEX [ VERBOSE ] object.
Actually, my first WIP version of patch added VERBOSE word at before
type of object.
I'm feeling difficult about that the position of VERBOSE word in
REINDEX statement.
The way that FORCE was added to REINDEX was poorly thought out; let's not
double down on that with another option added without any consideration
for future expansion. I'd be happier if we adopted something similar to
the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in
parentheses.
I understood.
I'm imagining new REINDEX syntax are followings.
- REINDEX (INDEX, VERBOSE) hoge_idx;
- REINDEX (TABLE) hoge_table;
i.g., I will add following syntax format,
REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )
name [FORCE];
Thought?
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
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Mon, Feb 2, 2015 at 8:31 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Attached patch adds VERBOSE option to REINDEX commands. Please give me feedbacks. This could provide useful feedback to users. Now, I think that it may be better to provide the keyword VERBOSE before the type of object reindexed as REINDEX [ VERBOSE ] object. In any case, at quick sight, the table completion for REINDEX is broken with your patch because by typing REINDEX VERBOSE you would show the list of objects and once again VERBOSE. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Mon, Feb 2, 2015 at 8:31 PM, Sawada Masahiko sawada.m...@gmail.com wrote: Attached patch adds VERBOSE option to REINDEX commands. Please give me feedbacks. This could provide useful feedback to users. Thanks. Now, I think that it may be better to provide the keyword VERBOSE before the type of object reindexed as REINDEX [ VERBOSE ] object. Actually, my first WIP version of patch added VERBOSE word at before type of object. I'm feeling difficult about that the position of VERBOSE word in REINDEX statement. In any case, at quick sight, the table completion for REINDEX is broken with your patch because by typing REINDEX VERBOSE you would show the list of objects and once again VERBOSE. I have also rebased the tab-completion source, I think it's not happen. In my environment, it does not show list of object and VERBOSE again after typing REINDEX VERBOSE. 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
[HACKERS] Proposal : REINDEX xxx VERBOSE
Hi all, Attached patch adds VERBOSE option to REINDEX commands. The another maintaining commands(VACUUM FULL, CLUSTER) has VERBOSE option, but REINDEX has not been had it. Examples is following, - REINDEX TABLE [postgres][5432](1)=# REINDEX TABLE VERBOSE hoge; INFO: index hoge_idx was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.02 sec. INFO: index hoge2_idx was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. REINDEX - REINDEX SCHEMA [postgres][5432](1)=# REINDEX SCHEMA VERBOSE s; INFO: index hoge_idx was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. INFO: index hoge2_idx was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. INFO: indexes of whole table s.hoge were reindexed REINDEX Please give me feedbacks. Regards, --- Sawada Masahiko 000_reindex_verbose_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
Sawada Masahiko sawada.m...@gmail.com writes: On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier michael.paqu...@gmail.com wrote: Now, I think that it may be better to provide the keyword VERBOSE before the type of object reindexed as REINDEX [ VERBOSE ] object. Actually, my first WIP version of patch added VERBOSE word at before type of object. I'm feeling difficult about that the position of VERBOSE word in REINDEX statement. The way that FORCE was added to REINDEX was poorly thought out; let's not double down on that with another option added without any consideration for future expansion. I'd be happier if we adopted something similar to the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in parentheses. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
