Re: Proposal : REINDEX xxx VERBOSE

On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello, I have some trivial comments about the latest patch.
> >
> > At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko <
sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBxPCpPvKQmvJMUh+p=
2pfAu03gKJQ2R2zY47XHsH205Q(at)mail(dot)gmail(dot)com>
> > sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <
Jim(dot)Nasby(at)bluetreble(dot)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