On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada.m...@gmail.com> wrote: > > On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI > <horiguchi.kyot...@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.m...@gmail.com> wrote in <CAD21AoBxPCpPvKQmvJMUh+p= 2pfau03gkjq2r2zy47xhsh2...@mail.gmail.com> > > sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby < jim.na...@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, -- 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