Re: Proposal : REINDEX xxx VERBOSE - Mailing list pgsql-hackers

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <<a
href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>>wrote:<br />><br />> On Fri, Mar 13, 2015 at
5:10PM, Kyotaro HORIGUCHI<br />> <<a
href="mailto:horiguchi.kyotaro@lab.ntt.co.jp">horiguchi.kyotaro@lab.ntt.co.jp</a>>wrote:<br />> > Hello, I
havesome trivial comments about the latest patch.<br />> ><br />> > At Thu, 12 Mar 2015 21:15:14 +0900,
SawadaMasahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>> wrote in
<CAD21AoBxPCpPvKQmvJMUh+p=<a
href="mailto:2pfAu03gKJQ2R2zY47XHsH205Q@mail.gmail.com">2pfAu03gKJQ2R2zY47XHsH205Q@mail.gmail.com</a>><br/>> >
sawada.mshk>On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <<a
href="mailto:Jim.Nasby@bluetreble.com">Jim.Nasby@bluetreble.com</a>>wrote:<br />> >> >>> >Are
theparenthesis necessary? No other WITH option requires them, other<br />> >> >>> >than create
table/matview(COPY doesn't actually require them).<br />> >> >>> ><br />> >> >><br
/>>>> >> I was imagining EXPLAIN syntax.<br />> >> >> Is there some possibility of
supportingmultiple options for REINDEX<br />> >> >> command in future?<br />> >> >> If
thereis, syntax will be as follows, REINDEX { INDEX | ... } name<br />> >> >> WITH VERBOSE, XXX, XXX;<br
/>>>> >> I thought style with parenthesis is better than above style.<br />> >> ><br />>
>>><br />> >> > The thing is, ()s are actually an odd-duck. Very little supports it, and<br />>
>>> while COPY allows it they're not required. EXPLAIN is a different story,<br />> >> > because
that'snot WITH; we're actually using () *instead of* WITH.<br />> >> ><br />> >> > So because
almostall commands that use WITH doen't even accept (), I don't<br />> >> > think this should either. It
certainlyshouldn't require them, because<br />> >> > unlike EXPLAIN, there's no need to require them.<br
/>>>> ><br />> >><br />> >> I understood what your point is.<br />> >> Attached
patchis changed syntax, it does not have parenthesis.<br />> ><br />> > As I looked into the code to find
whatthe syntax would be, I<br />> > found some points which would be better to be fixed.<br />> ><br />>
>In gram.y the options is a list of cstring but it is not necesary<br />> > to be a list because there's only
onekind of option now.<br />> ><br />> > If you prefer it to be a list, I have a comment for the way to<br
/>>> make string list in gram.y. You stored bare cstring in the<br />> > options list but I think it is not
thepreferable form. I suppose<br />> > the followings are preferable. Corresponding fixes are needed in<br />>
>ReindexTable, ReindexIndex, ReindexMultipleTables.<br />> ><br />> >     $ =
list_make1(makeString($1);<br/>> >  ....<br />> >     $ = lappend($1, list_make1(makeString($3));<br />>
><br/>> ><br />> > In equalfuncs.c, _equalReindexStmt forgets to compare the member<br />> >
options._copyReindexStmt also forgets to copy it. The way you<br />> > constructed the options list prevents them
fromdoing their jobs<br />> > using prepared methods. Comparing and copying the member "option"<br />> > is
neededeven if it becomes a simple string.<br />> ><br />><br />> I revised patch, and changed gram.y as I
don'tuse the list.<br />> So this patch adds new syntax,<br />> REINDEX { INDEX | ... } name WITH VERBOSE;<br
/>><br/>> Also documentation is updated.<br />> Please give me feedbacks.<br />><br /><br /></div><div
class="gmail_extra">Somenotes:<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">1) There are a
trailingspace in src/backend/parser/gram.y:<br /><br />-           | REINDEX DATABASE name opt_force<br />+           |
REINDEXreindex_target_multitable name WITH opt_verbose<br />                {<br />                    ReindexStmt *n =
makeNode(ReindexStmt);<br/>-                   n->kind = REINDEX_OBJECT_DATABASE;<br />+                  
n->kind= $2;<br />                    n->name = $3;<br />                    n->relation = NULL;<br
/>+                  n->verbose = $5;<br />                    $$ = (Node *)n;<br />                }<br />       
;<br/></div><div class="gmail_extra"><br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">2) The
documentationwas updated and is according the behaviour.<br /><br /></div><div class="gmail_extra"><br />3) psql
autocompleteis ok.<br /><br /></div><div class="gmail_extra"><br />4) Lack of regression tests. I think you should add
someregression like that:<br /><br />fabrizio=# \set VERBOSITY terse<br />fabrizio=# create table reindex_verbose(id
integerprimary key);<br />CREATE TABLE<br />fabrizio=# reindex table reindex_verbose with verbose;<br />INFO:  index
"reindex_verbose_pkey"was reindexed.<br />REINDEX<br /><br /></div><div class="gmail_extra"><br />5) Code style and
organizationis ok<br /><br /></div><div class="gmail_extra"><br />6) You should add the new field
ReindexStmt->verboseto src/backend/nodes/copyfuncs.c<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra"><br/>Regards,<br /></div><div class="gmail_extra"><br /><br />--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>