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.
Regards,
-------
Sawada Masahiko