Re: Proposal : REINDEX xxx VERBOSE

On Sun, May 10, 2015 at 2:23 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>>
>> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>>
>>>
>>> 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.
>> Just one last note. IMHO we should add a regression to
>> src/bin/scripts/090_reindexdb.pl.
>>
>
> Thank you for your patch!
> (Sorry for attaching the patch still has compile error..)
>
> - 000_reindex_verbose_v13.patch
> Looks good to me.
>
> - 001_reindexdb_verbose_option_v1.patch
> I noticed a bug in reindexdb patch, so fixed version is attached.
> The regression test for reindexdb is added as well.

Pushed.

Regards,

--
Fujii Masao