Re: Proposal : REINDEX xxx VERBOSE

On Tue, Apr 7, 2015 at 9:32 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada.mshk@gmail.com>
> wrote:
>>
>> On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
>> <horiguchi.kyotaro@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.mshk@gmail.com> wrote in
>> > <CAD21AoBxPCpPvKQmvJMUh+p=2pfAu03gKJQ2R2zY47XHsH205Q@mail.gmail.com>
>> > sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby
>> > <Jim.Nasby@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