Re: Proposal : REINDEX xxx VERBOSE

On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
> > <javascript:;>> wrote:
> >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com
> > <javascript:;>> wrote:
> >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> > <javascript:;>> wrote:
> >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
> >>>> <sawada(dot)mshk(at)gmail(dot)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,

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