Re: Proposal : REINDEX xxx VERBOSE

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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center