Обсуждение: Proposal : REINDEX xxx VERBOSE
Hi all, Attached patch adds VERBOSE option to REINDEX commands. The another maintaining commands(VACUUM FULL, CLUSTER) has VERBOSE option, but REINDEX has not been had it. Examples is following, - REINDEX TABLE [postgres][5432](1)=# REINDEX TABLE VERBOSE hoge; INFO: index "hoge_idx" was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.02 sec. INFO: index "hoge2_idx" was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. REINDEX - REINDEX SCHEMA [postgres][5432](1)=# REINDEX SCHEMA VERBOSE s; INFO: index "hoge_idx" was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. INFO: index "hoge2_idx" was reindexed. DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec. INFO: indexes of whole table "s.hoge" were reindexed REINDEX Please give me feedbacks. Regards, ------- Sawada Masahiko
Вложения
On Mon, Feb 2, 2015 at 8:31 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > Attached patch adds VERBOSE option to REINDEX commands. > Please give me feedbacks. This could provide useful feedback to users. Now, I think that it may be better to provide the keyword VERBOSE before the type of object reindexed as REINDEX [ VERBOSE ] object. In any case, at quick sight, the table completion for REINDEX is broken with your patch because by typing REINDEX VERBOSE you would show the list of objects and once again VERBOSE. -- Michael
On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Feb 2, 2015 at 8:31 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> Attached patch adds VERBOSE option to REINDEX commands. >> Please give me feedbacks. > This could provide useful feedback to users. Thanks. > Now, I think that it may > be better to provide the keyword VERBOSE before the type of object > reindexed as REINDEX [ VERBOSE ] object. Actually, my first WIP version of patch added VERBOSE word at before type of object. I'm feeling difficult about that the position of VERBOSE word in REINDEX statement. > In any case, at quick sight, > the table completion for REINDEX is broken with your patch because by > typing REINDEX VERBOSE you would show the list of objects and once > again VERBOSE. I have also rebased the tab-completion source, I think it's not happen. In my environment, it does not show list of object and VERBOSE again after typing REINDEX VERBOSE. Regards, ------- Sawada Masahiko
Sawada Masahiko <sawada.mshk@gmail.com> writes: > On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> Now, I think that it may >> be better to provide the keyword VERBOSE before the type of object >> reindexed as REINDEX [ VERBOSE ] object. > Actually, my first WIP version of patch added VERBOSE word at before > type of object. > I'm feeling difficult about that the position of VERBOSE word in > REINDEX statement. The way that FORCE was added to REINDEX was poorly thought out; let's not double down on that with another option added without any consideration for future expansion. I'd be happier if we adopted something similar to the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in parentheses. regards, tom lane
On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Sawada Masahiko <sawada.mshk@gmail.com> writes: >> On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> Now, I think that it may >>> be better to provide the keyword VERBOSE before the type of object >>> reindexed as REINDEX [ VERBOSE ] object. > >> Actually, my first WIP version of patch added VERBOSE word at before >> type of object. >> I'm feeling difficult about that the position of VERBOSE word in >> REINDEX statement. > > The way that FORCE was added to REINDEX was poorly thought out; let's not > double down on that with another option added without any consideration > for future expansion. I'd be happier if we adopted something similar to > the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in > parentheses. > I understood. I'm imagining new REINDEX syntax are followings. - REINDEX (INDEX, VERBOSE) hoge_idx; - REINDEX (TABLE) hoge_table; i.g., I will add following syntax format, REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] ) name [FORCE]; Thought? Regards, ------- Sawada Masahiko
<div dir="ltr"><div class="gmail_extra"><br />On Tue, Feb 3, 2015 at 9:09 AM, Sawada Masahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>>wrote:<br />><br />> On Tue, Feb 3, 2015 at 12:32AM, Tom Lane <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>> wrote:<br />> > Sawada Masahiko<<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>> writes:<br />> >> On Mon, Feb 2,2015 at 9:21 PM, Michael Paquier<br />> >> <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />> >>> Now, I think thatit may<br />> >>> be better to provide the keyword VERBOSE before the type of object<br />> >>>reindexed as REINDEX [ VERBOSE ] object.<br />> ><br />> >> Actually, my first WIP version ofpatch added VERBOSE word at before<br />> >> type of object.<br />> >> I'm feeling difficult about thatthe position of VERBOSE word in<br />> >> REINDEX statement.<br />> ><br />> > The way that FORCEwas added to REINDEX was poorly thought out; let's not<br />> > double down on that with another option addedwithout any consideration<br />> > for future expansion. I'd be happier if we adopted something similar to<br/>> > the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in<br />> > parentheses.<br/>> ><br />><br />> I understood.<br />> I'm imagining new REINDEX syntax are followings.<br/>> - REINDEX (INDEX, VERBOSE) hoge_idx;<br />> - REINDEX (TABLE) hoge_table;<br />><br />> i.g.,I will add following syntax format,<br />> REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )<br/>> name [FORCE];<br />><br />> Thought?<br />><br /><br /></div><div class="gmail_extra">I don't think thekeyworks INDEX, TABLE, SCHEMA, SYSTEM and DATABASE are options... they are part of the command IMHO.<br /><br /></div><divclass="gmail_extra">Maybe something like:<br /><br /><br />REINDEX { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE} [( [ FORCE ], [VERBOSE] ) ] name;<br /><br /></div><div class="gmail_extra">And maintain the old syntax for compatibilityof course.<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br /><br />--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<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>
Sawada Masahiko <sawada.mshk@gmail.com> writes: > On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> The way that FORCE was added to REINDEX was poorly thought out; let's not >> double down on that with another option added without any consideration >> for future expansion. I'd be happier if we adopted something similar to >> the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in >> parentheses. > I understood. > I'm imagining new REINDEX syntax are followings. > - REINDEX (INDEX, VERBOSE) hoge_idx; > - REINDEX (TABLE) hoge_table; > i.g., I will add following syntax format, > REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] ) > name [FORCE]; Well, the object type is not an optional part of the command. It's *necessary*. I was thinking more like REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] option := FORCE | VERBOSE We'd still keep the historical syntax where you can write FORCE outside parens, but it'd be deprecated. Where to insert the parenthesized option list is a judgment call, but I'd lean to keeping it at the end where FORCE used to be. regards, tom lane
On 2015-02-03 10:20:03 -0500, Tom Lane wrote: > Sawada Masahiko <sawada.mshk@gmail.com> writes: > > On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> The way that FORCE was added to REINDEX was poorly thought out; let's not > >> double down on that with another option added without any consideration > >> for future expansion. I'd be happier if we adopted something similar to > >> the modern syntax for VACUUM and EXPLAIN, ie, comma-separated options in > >> parentheses. > > > I understood. > > I'm imagining new REINDEX syntax are followings. > > - REINDEX (INDEX, VERBOSE) hoge_idx; > > - REINDEX (TABLE) hoge_table; > > > i.g., I will add following syntax format, > > REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] ) > > name [FORCE]; > > Well, the object type is not an optional part of the command. It's > *necessary*. I was thinking more like > > REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] > > option := FORCE | VERBOSE > > We'd still keep the historical syntax where you can write FORCE outside > parens, but it'd be deprecated. Why would we allow force inside the parens, given it's a backward compat only thing afaik? Don't get me wrong, I'm not at all against a extensible syntax, I just don't see a point in further cargo culting FORCE. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes: > On 2015-02-03 10:20:03 -0500, Tom Lane wrote: >> Well, the object type is not an optional part of the command. It's >> *necessary*. I was thinking more like >> >> REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] >> >> option := FORCE | VERBOSE >> >> We'd still keep the historical syntax where you can write FORCE outside >> parens, but it'd be deprecated. > Why would we allow force inside the parens, given it's a backward compat > only thing afaik? Don't get me wrong, I'm not at all against a > extensible syntax, I just don't see a point in further cargo culting > FORCE. Ah, I'd forgotten that that option was now a no-op. Yeah, there's no reason to support it in the new syntax. regards, tom lane
On 2/3/15 9:20 AM, Tom Lane wrote: >> >i.g., I will add following syntax format, >> >REINDEX ( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] ) >> >name [FORCE]; > Well, the object type is not an optional part of the command. It's > *necessary*. I was thinking more like > > REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] VACUUM puts the options before the table name, so ISTM it'd be best to keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or REINDEX {INDEX | ...} (options). -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@bluetreble.com> writes: > On 2/3/15 9:20 AM, Tom Lane wrote: >> Well, the object type is not an optional part of the command. It's >> *necessary*. I was thinking more like >> REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] > VACUUM puts the options before the table name, so ISTM it'd be best to > keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or > REINDEX {INDEX | ...} (options). Well, I really really don't like the first of those. IMO the command name is "REINDEX INDEX" etc, so sticking something in the middle of that is bogus. regards, tom lane
<div dir="ltr"><div class="gmail_extra"><br />On Tue, Feb 3, 2015 at 8:26 PM, Jim Nasby <<a href="mailto:Jim.Nasby@bluetreble.com">Jim.Nasby@bluetreble.com</a>>wrote:<br />><br />> On 2/3/15 9:20 AM, TomLane wrote:<br />>>><br />>>> >i.g., I will add following syntax format,<br />>>> >REINDEX( { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } , [VERBOSE] )<br />>>> >name [FORCE];<br />>><br/>>> Well, the object type is not an optional part of the command. It's<br />>> *necessary*. Iwas thinking more like<br />>><br />>> REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ]<br/>><br />><br />> VACUUM puts the options before the table name, so ISTM it'd be best to keep that with REINDEX.Either REINDEX (options) {INDEX | ...} or REINDEX {INDEX | ...} (options).<br />> <br /><br /></div><div class="gmail_extra">Makessense... +1<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><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>
On 2/3/15 5:08 PM, Tom Lane wrote: > Jim Nasby <Jim.Nasby@bluetreble.com> writes: >> On 2/3/15 9:20 AM, Tom Lane wrote: >>> Well, the object type is not an optional part of the command. It's >>> *necessary*. I was thinking more like >>> REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] > >> VACUUM puts the options before the table name, so ISTM it'd be best to >> keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or >> REINDEX {INDEX | ...} (options). > > Well, I really really don't like the first of those. IMO the command name > is "REINDEX INDEX" etc, so sticking something in the middle of that is > bogus. Actually, is there a reason we can't just accept all 3? Forcing people to remember exact ordering of options has always struck me as silly. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
Jim Nasby <Jim.Nasby@BlueTreble.com> writes: > On 2/3/15 5:08 PM, Tom Lane wrote: >> Jim Nasby <Jim.Nasby@bluetreble.com> writes: >>> VACUUM puts the options before the table name, so ISTM it'd be best to >>> keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or >>> REINDEX {INDEX | ...} (options). >> Well, I really really don't like the first of those. IMO the command name >> is "REINDEX INDEX" etc, so sticking something in the middle of that is >> bogus. > Actually, is there a reason we can't just accept all 3? Forcing people > to remember exact ordering of options has always struck me as silly. And that's an even worse idea. Useless "flexibility" in syntax tends to lead to unfortunate consequences like having to reserve keywords. regards, tom lane
On Wed, Feb 4, 2015 at 2:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jim Nasby <Jim.Nasby@BlueTreble.com> writes: >> On 2/3/15 5:08 PM, Tom Lane wrote: >>> Jim Nasby <Jim.Nasby@bluetreble.com> writes: >>>> VACUUM puts the options before the table name, so ISTM it'd be best to >>>> keep that with REINDEX. Either REINDEX (options) {INDEX | ...} or >>>> REINDEX {INDEX | ...} (options). > >>> Well, I really really don't like the first of those. IMO the command name >>> is "REINDEX INDEX" etc, so sticking something in the middle of that is >>> bogus. > >> Actually, is there a reason we can't just accept all 3? Forcing people >> to remember exact ordering of options has always struck me as silly. > > And that's an even worse idea. Useless "flexibility" in syntax tends to > lead to unfortunate consequences like having to reserve keywords. > As per discussion, it seems to good with REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] or REINDEX { INDEX | TABLE | etc } [ (option [, optoin ...] ) ] name i.g., the options of reindex(VERBOSE and FORCE) are put at before or after object name. Because other maintenance command put option at before object name, I think the latter is better. Regards, ------- Sawada Masahiko
Hello, > As per discussion, it seems to good with > REINDEX { INDEX | TABLE | etc } name [ ( option [, option ...] ) ] > or > REINDEX { INDEX | TABLE | etc } [ (option [, optoin ...] ) ] name > i.g., the options of reindex(VERBOSE and FORCE) are put at before or > after object name. > > Because other maintenance command put option at before object name, I > think the latter is better. The phrase "{INDEX | TABLE |..} name" seems to me indivisible as target specification. IMHO, the options for VACUUM and so is placed *just after* command name, not *before* the target. If this is right, the syntax would be like this. REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name What do you think about this? regares, -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: > The phrase "{INDEX | TABLE |..} name" seems to me indivisible as > target specification. IMHO, the options for VACUUM and so is > placed *just after* command name, not *before* the target. > If this is right, the syntax would be like this. > REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name > What do you think about this? I think this is wrong and ugly. INDEX etc are part of the command name. regards, tom lane
Tom Lane-2 wrote > Kyotaro HORIGUCHI < > horiguchi.kyotaro@.co > > writes: >> The phrase "{INDEX | TABLE |..} name" seems to me indivisible as >> target specification. IMHO, the options for VACUUM and so is >> placed *just after* command name, not *before* the target. > >> If this is right, the syntax would be like this. > >> REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name > >> What do you think about this? > > I think this is wrong and ugly. INDEX etc are part of the command name. I can argue either position... I lean toward those not being part of the command name because: The documentation lists only "REINDEX" as a command while "DROP" gets a separate entry for each type of object it is able to drop; mainly because the behavior of each command could/does differ based upon the target while REINDEX will still simply cause indexes to be rebuilt and the modifier's purposes is to aid in the selection of target indexes. REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | DATABASE | SYSTEM } name That said, the entire notes section is written like the writer also believed that "REINDEX INDEX" is a command in its own right... VACUUM is a good comparison command and, besides, putting VERBOSE after the entire thing just doesn't seem right - though that is the only other option that would work for me. When you read other commands with pre and post options the wording usually flows reasonably well (IF EXISTS being, for me, an exception - it reads better after the name, not before, but I digress...). REINDEX ( VERBOSE ) /target/ reads well to me; I'm already sold that "TABLE name" and "INDEX name" are target specifiers. David J. -- View this message in context: http://postgresql.nabble.com/Proposal-REINDEX-xxx-VERBOSE-tp5836377p5836782.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
On Wed, Feb 4, 2015 at 8:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: >> The phrase "{INDEX | TABLE |..} name" seems to me indivisible as >> target specification. IMHO, the options for VACUUM and so is >> placed *just after* command name, not *before* the target. > >> If this is right, the syntax would be like this. > >> REINDEX [ (option [, option ...] ) ] {INDEX | TABLE | etc } name > >> What do you think about this? > > I think this is wrong and ugly. INDEX etc are part of the command name. I don't think so. I think they're part of the target. We have one manual page is for REINDEX, not five separate reference pages for REINDEX INDEX, REINDEX TABLE, REINDEX SCHEMA, REINDEX DATABASE, and REINDEX SYSTEM. If we really wanted to, we could probably even support this: REINDEX INDEX foo, TABLE bar, TABLE baz; We've got a mix of styles for extensible options right now: EXPLAIN [ ( option [, ...] ) ] statement COPY table_name [ ( column_name [, ...] ) ] FROM { 'filename' | PROGRAM 'command' | STDIN } [ [ WITH ] ( option [, ...]) ] VACUUM [ ( { FULL | FREEZE | VERBOSE | ANALYZE } [, ...] ) ] [ table_name [ (column_name [, ...] ) ] ] So COPY puts the options at the very end, but EXPLAIN and VACUUM put them right after the command name. I prefer the latter style and would vote to adopt it here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > We've got a mix of styles for extensible options right now: That we do. > So COPY puts the options at the very end, but EXPLAIN and VACUUM put > them right after the command name. I prefer the latter style and > would vote to adopt it here. Meh. Options-at-the-end seems by far the most sensible style to me. The options-right-after-the-keyword style is a mess, both logically and from a parsing standpoint, and the only reason we have it at all is historical artifact (ask Bruce about the origins of VACUUM ANALYZE over a beer sometime). Still, I can't help noticing that I'm being outvoted. I'll shut up now. regards, tom lane
On 2/5/15 12:01 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> We've got a mix of styles for extensible options right now: > > That we do. > >> So COPY puts the options at the very end, but EXPLAIN and VACUUM put >> them right after the command name. I prefer the latter style and >> would vote to adopt it here. > > Meh. Options-at-the-end seems by far the most sensible style to me. > The options-right-after-the-keyword style is a mess, both logically > and from a parsing standpoint, and the only reason we have it at all > is historical artifact (ask Bruce about the origins of VACUUM ANALYZE > over a beer sometime). I suspect at least some of this stems from how command line programs tend to process options before arguments. I tend to agree with you Tom, but I think what's more important is that we're consistent. COPY is already a bit of an oddball because it uses WITH, but both EXPLAIN and VACUUM use parenthesis immediately after the first verb. Introducing a parenthesis version that goes at the end instead of the beginning is just going to make this worse. If we're going to take a stand on this, we need to do it NOW, before we have even more commands that use (). I know you were worried about accepting options anywhere because it leads to reserved words, but perhaps we could support it just for EXPLAIN and VACUUM, and then switch to trailing options if people think that would be better. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
Hello, I had a look on gram.y and found other syntaxes using WITH option clause. At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DBBCC9.1020206@BlueTreble.com> > I suspect at least some of this stems from how command line programs > tend to process options before arguments. I tend to agree with you > Tom, but I think what's more important is that we're consistent. COPY > is already a bit of an oddball because it uses WITH, but both EXPLAIN > and VACUUM use parenthesis immediately after the first > verb. Introducing a parenthesis version that goes at the end instead > of the beginning is just going to make this worse. > > If we're going to take a stand on this, we need to do it NOW, before > we have even more commands that use (). > > I know you were worried about accepting options anywhere because it > leads to reserved words, but perhaps we could support it just for > EXPLAIN and VACUUM, and then switch to trailing options if people > think that would be better. I agree with the direction, but I see two issues here; how many syntax variants we are allowed to have for one command at a time, and how far we should/may extend the unified options syntax on other commands. Let me put the issues aside for now, VACUUM can have trailing options naturally but it seems to change, but, IMHO, EXPLAIN should have the target statement at the tail end. Are you thinking of syntaxes like following? VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)] | VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)] | VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})] REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)] EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] <statement> For concrete examples, the lines prefixed by asterisk are in new syntax. VACUUM FULL table1;VACUUM ANALYZE table1 (col1);VACUUM (ANALYZE, VERBOSE) table1 (col1); *VACUUM table1 WITH (FREEZE on) *VACUUM table1 (cola) WITH (ANALYZE) *VACUUM table1 WITH (ANALYZE) *VACUUM table1 (FREEZE on) The fifth example looks quite odd. REINDEX INDEX index1 FORCE; *REINDEX TABLE table1 WITH (VERBOSE on); *REINDEX TABLE table1 (VERBOSE on, FORCE on);EXPLAIN (ANALYZE) SELECT 1; *EXPLAIN WITH (ANALYZE) SELECT 1; The WITH looks a bit uneasy.. COPY table1 FROM 'file.txt' WITH (FORMAT csv); Returning to the second issue, the following statements have option list (or single option) preceded (or not preceded) by the word WITH. The prefixing dollar sign indicates that the syntax is of SQL standard according to the PostgreSQL Documentation. Asterisk indicates that the line shows the syntax if the new policy is applied. Other few statements like DECLARE looks using WITH as a part of an idiom. CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; This is similar to EXPLAIN in the sense that a query follows it, but this syntax can have the second WITH follows by DATA. CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; *CREATE EXTENSION ext1 WITH (SCHEMA s1, VERSION v1, FROM over); This seems to fit the unification. CREATE ROLE role WITH LOGIN;CREATE ROLE role SUPERUSER, LOGIN; $CREATE ROLE role WITH ADMIN admin; *CREATE ROLE role WITH (SUPERUSER, LOGIN); *CREATE ROLE role (SUPERUSER, LOGIN); This seems meaninglessly too complecated. GRANT .... WITH GRANT OPTION; *GRANT .... WITH (GRANT on); Mmm. Seems no reasoning... CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; *CREATE VIEW v1 AS qry WITH (CASCADED_CHECK); Wired syntax? ALTER DATABASE db1 WITH CONNECTION LIMIT 50; *ALTER DATABASE db1 WITH (CONNECTION_LIMIT 50); Hardly looks reasonable.. $DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; This cannot have another style. Mmm... I'm at a loss what is desirable.. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On 2/16/15 9:43 PM, Kyotaro HORIGUCHI wrote: > Hello, I had a look on gram.y and found other syntaxes using WITH > option clause. > > At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby<Jim.Nasby@BlueTreble.com> wrote in<54DBBCC9.1020206@BlueTreble.com> >> >I suspect at least some of this stems from how command line programs >> >tend to process options before arguments. I tend to agree with you >> >Tom, but I think what's more important is that we're consistent. COPY >> >is already a bit of an oddball because it uses WITH, but both EXPLAIN >> >and VACUUM use parenthesis immediately after the first >> >verb. Introducing a parenthesis version that goes at the end instead >> >of the beginning is just going to make this worse. >> > >> >If we're going to take a stand on this, we need to do it NOW, before >> >we have even more commands that use (). >> > >> >I know you were worried about accepting options anywhere because it >> >leads to reserved words, but perhaps we could support it just for >> >EXPLAIN and VACUUM, and then switch to trailing options if people >> >think that would be better. > I agree with the direction, but I see two issues here; how many > syntax variants we are allowed to have for one command at a time, > and how far we should/may extend the unified options syntax on > other commands. > > > Let me put the issues aside for now, VACUUM can have trailing > options naturally but it seems to change, but, IMHO, EXPLAIN > should have the target statement at the tail end. Are you > thinking of syntaxes like following? > > VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)] > | VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)] > | VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})] > > REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)] > > EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] <statement> > > For concrete examples, the lines prefixed by asterisk are in new > syntax. If I could choose only one for explain, I would find it easier to be up front. That way you do the explain part on one line and just paste the query after that. > VACUUM FULL table1; > VACUUM ANALYZE table1 (col1); > VACUUM (ANALYZE, VERBOSE) table1 (col1); > *VACUUM table1 WITH (FREEZE on) > *VACUUM table1 (cola) WITH (ANALYZE) > *VACUUM table1 WITH (ANALYZE) > *VACUUM table1 (FREEZE on) > > The fifth example looks quite odd. I don't think we need to allow both () and WITH... I'd say one or the other, preferably (). -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
> VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)] > | VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)] > | VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})] > > REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)] > > EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] <statement> > I don't think "(OptionName [bool])" style like "(VERBOSE on, FORCE on)" is needed for REINDEX command. EXPLAIN command has such option style because it has the FORMAT option can have value excepting ON/TRUE or OFF/FALSE.(e.g., TEXT, XML) But the value of REINDEX command option can have only ON or OFF. I think the option name is good enough. Next, regarding of the location of such option, the several maintenance command like CLUSTER, VACUUM has option at immediately after command name. From consistency perspective, I tend to agree with Robert to put option at immediately after command name as follows. REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name; Btw how long will the FORCE command available? Regards, ------- Sawada Masahiko
Hello, I showed an extreme number of examples to include *almost of all* variations of existing syntax of option specification. And showed what if all variations could be used for all commands. It was almost a mess. Sorry for the confusion. I think the issues at our hands are, - Options location: at-the-end or right-after-the-keyword? - FORCE options to be removed? - Decide whether to allow bare word option if the options are to be located right after the keyword. Optinions or thoughts? ==== Rethinking from here. At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DBBCC9.1020206@BlueTreble.com> > On 2/5/15 12:01 PM, Tom Lane wrote: ... > > Meh. Options-at-the-end seems by far the most sensible style to me. > > The options-right-after-the-keyword style is a mess, both logically > > and from a parsing standpoint, and the only reason we have it at all > > is historical artifact (ask Bruce about the origins of VACUUM ANALYZE > > over a beer sometime). ... > I know you were worried about accepting options anywhere because it > leads to reserved words, but perhaps we could support it just for > EXPLAIN and VACUUM, and then switch to trailing options if people > think that would be better. According to the above discussion, VACUUM and REINDEX should have trailing options. Tom seems (to me) suggesting that SQL-style (bare word preceded by WITH) options and Jim suggesting '()' style options? (Anyway VACUUM gets the *third additional* option sytle, but it is the different discussion from this) VACUUM [tname [(cname, ...)]] [({FULL [bool]|FREEZE | [bool]|...})] =# VACUUM t1 (FULL, FREEZE); VACUUM [tname [(cname, ...)]] [WITH [FULL [bool]]|[FREEZE | [bool]|...] =# VACUUM t1 WITH FULL; IMHO, we are so accustomed to call by the names "VACUUM FULL" or "VACUUM FREEZE" that the both of them look a bit uneasy. If the new syntax above is added, REINDEX should have *only* the trailing style. REINDEX [{INDEX|TABLE|...}] name [(VERBOSE [bool]|...)] =# REINDEX TABLE t1 (VERBOSE); REINDEX [{INDEX|TABLE|...}] name [WITH {VERBOSE [bool]|...}] =# REINDEX INDEX i_t1_pkey WITH VERBOSE; Also, both of them seems to be unintuitive.. EXPLAIN.. it seems to be preferred to be as it is.. As the result, it seems the best way to go on the current syntax for all of those commands. Optinions? At Wed, 18 Feb 2015 23:58:15 +0900, Sawada Masahiko <sawada.mshk@gmail.com> wrote in <CAD21AoBkjndQAq2Z-WbScMdHoX257BJj6sUYthwwFs2iL8YDhQ@mail.gmail.com> > From consistency perspective, I tend to agree with Robert to put > option at immediately after command name as follows. > REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name; I don't object against it if you prefer it. The remaining issue is the choice between options-at-the-end or this options-right-after-the-keyword mentioned above. I prefer the more messy(:-) one.. > Btw how long will the FORCE command available? The options is obsolete since 7.4. I think it should have been fade away long since and it's the time to remove it. But once the ancient option removed, the above syntax looks a bit uneasy and the more messy syntax looks natural. REINDEX [VERBOSE] {INDEX | ...} name; That do you think about this? regards, At Tue, 17 Feb 2015 12:00:13 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54E381AD.1090409@BlueTreble.com> > > VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)] > > | VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)] > > | VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE > > | [bool]|...})] > > > > REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)] > > > > EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})] > > <statement> > > > > For concrete examples, the lines prefixed by asterisk are in new > > syntax. > > If I could choose only one for explain, I would find it easier to be > up front. That way you do the explain part on one line and just paste > the query after that. .. > > VACUUM FULL table1; > > VACUUM ANALYZE table1 (col1); > > VACUUM (ANALYZE, VERBOSE) table1 (col1); > > *VACUUM table1 WITH (FREEZE on) > > *VACUUM table1 (cola) WITH (ANALYZE) > > *VACUUM table1 WITH (ANALYZE) > > *VACUUM table1 (FREEZE on) > > > > The fifth example looks quite odd. > > I don't think we need to allow both () and WITH... I'd say one or the > other, preferably (). -- Kyotaro Horiguchi NTT Open Source Software Center
On Fri, Feb 20, 2015 at 12:24 PM, Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote: > Hello, > > I showed an extreme number of examples to include *almost of all* > variations of existing syntax of option specification. And showed > what if all variations could be used for all commands. It was > almost a mess. Sorry for the confusion. > > I think the issues at our hands are, > > - Options location: at-the-end or right-after-the-keyword? > > - FORCE options to be removed? > > - Decide whether to allow bare word option if the options are to > be located right after the keyword. > > Optinions or thoughts? > > ==== > Rethinking from here. > > At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby <Jim.Nasby@BlueTreble.com> wrote in <54DBBCC9.1020206@BlueTreble.com> >> On 2/5/15 12:01 PM, Tom Lane wrote: > ... >> > Meh. Options-at-the-end seems by far the most sensible style to me. >> > The options-right-after-the-keyword style is a mess, both logically >> > and from a parsing standpoint, and the only reason we have it at all >> > is historical artifact (ask Bruce about the origins of VACUUM ANALYZE >> > over a beer sometime). > ... >> I know you were worried about accepting options anywhere because it >> leads to reserved words, but perhaps we could support it just for >> EXPLAIN and VACUUM, and then switch to trailing options if people >> think that would be better. > > According to the above discussion, VACUUM and REINDEX should have > trailing options. Tom seems (to me) suggesting that SQL-style > (bare word preceded by WITH) options and Jim suggesting '()' > style options? (Anyway VACUUM gets the *third additional* option > sytle, but it is the different discussion from this) > > VACUUM [tname [(cname, ...)]] [({FULL [bool]|FREEZE | [bool]|...})] > > =# VACUUM t1 (FULL, FREEZE); > > VACUUM [tname [(cname, ...)]] [WITH [FULL [bool]]|[FREEZE | [bool]|...] > > =# VACUUM t1 WITH FULL; > > IMHO, we are so accustomed to call by the names "VACUUM FULL" or > "VACUUM FREEZE" that the both of them look a bit uneasy. > > > If the new syntax above is added, REINDEX should have *only* the > trailing style. > > REINDEX [{INDEX|TABLE|...}] name [(VERBOSE [bool]|...)] > > =# REINDEX TABLE t1 (VERBOSE); > > REINDEX [{INDEX|TABLE|...}] name [WITH {VERBOSE [bool]|...}] > > =# REINDEX INDEX i_t1_pkey WITH VERBOSE; > > Also, both of them seems to be unintuitive.. > > > EXPLAIN.. it seems to be preferred to be as it is.. > > > As the result, it seems the best way to go on the current syntax > for all of those commands. > > Optinions? > > > > At Wed, 18 Feb 2015 23:58:15 +0900, Sawada Masahiko <sawada.mshk@gmail.com> wrote in <CAD21AoBkjndQAq2Z-WbScMdHoX257BJj6sUYthwwFs2iL8YDhQ@mail.gmail.com> >> From consistency perspective, I tend to agree with Robert to put >> option at immediately after command name as follows. >> REINDEX [(VERBOSE | FORCE)] {INDEX | ...} name; > > I don't object against it if you prefer it. The remaining issue > is the choice between options-at-the-end or this > options-right-after-the-keyword mentioned above. I prefer the > more messy(:-) one.. > > >> Btw how long will the FORCE command available? > > The options is obsolete since 7.4. I think it should have been > fade away long since and it's the time to remove it. But once the > ancient option removed, the above syntax looks a bit uneasy and > the more messy syntax looks natural. > > > REINDEX [VERBOSE] {INDEX | ...} name; > > That do you think about this? > Thank you for summarizing them. I said right-after-the-keyword is looks good to me. But it's will be possible only if FORCE command is removed. REINDEX command has FORCE option at the end, so REINDEX probably should have options at the end. Regards, ------- Sawada Masahiko
On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>According to the above discussion, VACUUM and REINDEX should have >>trailing options. Tom seems (to me) suggesting that SQL-style >>(bare word preceded by WITH) options and Jim suggesting '()' >>style options? (Anyway VACUUM gets the*third additional* option >>sytle, but it is the different discussion from this) Well, almost everything does a trailing WITH. We need to either stick with that for consistency, or add leading () as an option to those WITH commands. Does anyone know why those are WITH? Is it ANSI? As a refresher, current commands are: VACUUM (ANALYZE, VERBOSE) table1 (col1); REINDEX INDEX index1 FORCE; COPY table1 FROM 'file.txt' WITH (FORMAT csv); CREATEMATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROMover; CREATE ROLE role WITH LOGIN; GRANT .... WITH GRANT OPTION; CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; ALTERDATABASE db1 WITH CONNECTION LIMIT 50; DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>> >>> According to the above discussion, VACUUM and REINDEX should have >>> trailing options. Tom seems (to me) suggesting that SQL-style >>> (bare word preceded by WITH) options and Jim suggesting '()' >>> style options? (Anyway VACUUM gets the*third additional* option >>> sytle, but it is the different discussion from this) > > > Well, almost everything does a trailing WITH. We need to either stick with > that for consistency, or add leading () as an option to those WITH commands. > > Does anyone know why those are WITH? Is it ANSI? > > As a refresher, current commands are: > > VACUUM (ANALYZE, VERBOSE) table1 (col1); > REINDEX INDEX index1 FORCE; > COPY table1 FROM 'file.txt' WITH (FORMAT csv); > CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; > CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; > CREATE ROLE role WITH LOGIN; > GRANT .... WITH GRANT OPTION; > CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; > ALTER DATABASE db1 WITH CONNECTION LIMIT 50; > DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; We have discussed about this option including FORCE option, but I think there are not user who want to use both FORCE and VERBOSE option at same time. Can we think and add new syntax without FORCE option while leaving current Reindex statement syntax? As prototype, I attached new version patch has the following syntax. REINDEX { INDEX | TABLE | ... } relname [ FORCE | VERBOSE]; Regards, ------- Sawada Masahiko
Вложения
On 3/2/15 10:58 AM, Sawada Masahiko wrote: > On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >> On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>>> >>>> According to the above discussion, VACUUM and REINDEX should have >>>> trailing options. Tom seems (to me) suggesting that SQL-style >>>> (bare word preceded by WITH) options and Jim suggesting '()' >>>> style options? (Anyway VACUUM gets the*third additional* option >>>> sytle, but it is the different discussion from this) >> >> >> Well, almost everything does a trailing WITH. We need to either stick with >> that for consistency, or add leading () as an option to those WITH commands. >> >> Does anyone know why those are WITH? Is it ANSI? >> >> As a refresher, current commands are: >> >> VACUUM (ANALYZE, VERBOSE) table1 (col1); >> REINDEX INDEX index1 FORCE; >> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; >> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >> CREATE ROLE role WITH LOGIN; >> GRANT .... WITH GRANT OPTION; >> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most consistent with everything else. Is there a problem with doing that? I know getting syntax is one of the hard parts of new features, but it seems like we reached consensus here... > We have discussed about this option including FORCE option, but I > think there are not user who want to use both FORCE and VERBOSE option > at same time. I find that very hard to believe... I would expect a primary use case for VERBOSE to be "I ran REINDEX, but it doesn't seem to have done anything... what's going on?" and that's exactly when you might want to use FORCE. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 3/2/15 10:58 AM, Sawada Masahiko wrote: >> >> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> >> wrote: >>> >>> On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>>>> >>>>> >>>>> According to the above discussion, VACUUM and REINDEX should have >>>>> trailing options. Tom seems (to me) suggesting that SQL-style >>>>> (bare word preceded by WITH) options and Jim suggesting '()' >>>>> style options? (Anyway VACUUM gets the*third additional* option >>>>> sytle, but it is the different discussion from this) >>> >>> >>> >>> Well, almost everything does a trailing WITH. We need to either stick >>> with >>> that for consistency, or add leading () as an option to those WITH >>> commands. >>> >>> Does anyone know why those are WITH? Is it ANSI? >>> >>> As a refresher, current commands are: >>> >>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>> REINDEX INDEX index1 FORCE; >>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; >>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>> CREATE ROLE role WITH LOGIN; >>> GRANT .... WITH GRANT OPTION; >>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; > > > BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most > consistent with everything else. Is there a problem with doing that? I know > getting syntax is one of the hard parts of new features, but it seems like > we reached consensus here... Attached is latest version patch based on Tom's idea as follows. REINDEX { INDEX | ... } name WITH ( options [, ...] ) > >> We have discussed about this option including FORCE option, but I >> think there are not user who want to use both FORCE and VERBOSE option >> at same time. > > > I find that very hard to believe... I would expect a primary use case for > VERBOSE to be "I ran REINDEX, but it doesn't seem to have done anything... > what's going on?" and that's exactly when you might want to use FORCE. > In currently code, nothing happens even if FORCE option is specified. This option completely exist for backward compatibility. But this patch add new syntax including FORCE option for now. Todo - tab completion - reindexdb command Regards, ------- Sawada Masahiko
Вложения
On 3/9/15 9:43 PM, Sawada Masahiko wrote: > On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >> On 3/2/15 10:58 AM, Sawada Masahiko wrote: >>> >>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> >>> wrote: >>>> >>>> On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>>>>> >>>>>> >>>>>> According to the above discussion, VACUUM and REINDEX should have >>>>>> trailing options. Tom seems (to me) suggesting that SQL-style >>>>>> (bare word preceded by WITH) options and Jim suggesting '()' >>>>>> style options? (Anyway VACUUM gets the*third additional* option >>>>>> sytle, but it is the different discussion from this) >>>> >>>> >>>> >>>> Well, almost everything does a trailing WITH. We need to either stick >>>> with >>>> that for consistency, or add leading () as an option to those WITH >>>> commands. >>>> >>>> Does anyone know why those are WITH? Is it ANSI? >>>> >>>> As a refresher, current commands are: >>>> >>>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>>> REINDEX INDEX index1 FORCE; >>>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH DATA; >>>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>>> CREATE ROLE role WITH LOGIN; >>>> GRANT .... WITH GRANT OPTION; >>>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; >> >> >> BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the most >> consistent with everything else. Is there a problem with doing that? I know >> getting syntax is one of the hard parts of new features, but it seems like >> we reached consensus here... > > Attached is latest version patch based on Tom's idea as follows. > REINDEX { INDEX | ... } name WITH ( options [, ...] ) Are the parenthesis necessary? No other WITH option requires them, other than create table/matview (COPY doesn't actually require them). >>> We have discussed about this option including FORCE option, but I >>> think there are not user who want to use both FORCE and VERBOSE option >>> at same time. >> >> >> I find that very hard to believe... I would expect a primary use case for >> VERBOSE to be "I ran REINDEX, but it doesn't seem to have done anything... >> what's going on?" and that's exactly when you might want to use FORCE. >> > > In currently code, nothing happens even if FORCE option is specified. > This option completely exist for backward compatibility. > But this patch add new syntax including FORCE option for now. I forgot that. There's no reason to support it with the new stuff then. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On Tue, Mar 10, 2015 at 5:05 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 3/9/15 9:43 PM, Sawada Masahiko wrote: >> >> On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> >> wrote: >>> >>> On 3/2/15 10:58 AM, Sawada Masahiko wrote: >>>> >>>> >>>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> >>>> wrote: >>>>> >>>>> >>>>> On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> According to the above discussion, VACUUM and REINDEX should have >>>>>>> trailing options. Tom seems (to me) suggesting that SQL-style >>>>>>> (bare word preceded by WITH) options and Jim suggesting '()' >>>>>>> style options? (Anyway VACUUM gets the*third additional* option >>>>>>> sytle, but it is the different discussion from this) >>>>> >>>>> >>>>> >>>>> >>>>> Well, almost everything does a trailing WITH. We need to either stick >>>>> with >>>>> that for consistency, or add leading () as an option to those WITH >>>>> commands. >>>>> >>>>> Does anyone know why those are WITH? Is it ANSI? >>>>> >>>>> As a refresher, current commands are: >>>>> >>>>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>>>> REINDEX INDEX index1 FORCE; >>>>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>>>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH >>>>> DATA; >>>>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>>>> CREATE ROLE role WITH LOGIN; >>>>> GRANT .... WITH GRANT OPTION; >>>>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>>>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>>>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; >>> >>> >>> >>> BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the >>> most >>> consistent with everything else. Is there a problem with doing that? I >>> know >>> getting syntax is one of the hard parts of new features, but it seems >>> like >>> we reached consensus here... >> >> >> Attached is latest version patch based on Tom's idea as follows. >> REINDEX { INDEX | ... } name WITH ( options [, ...] ) > > > 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. >>>> We have discussed about this option including FORCE option, but I >>>> think there are not user who want to use both FORCE and VERBOSE option >>>> at same time. >>> >>> >>> >>> I find that very hard to believe... I would expect a primary use case for >>> VERBOSE to be "I ran REINDEX, but it doesn't seem to have done >>> anything... >>> what's going on?" and that's exactly when you might want to use FORCE. >>> >> >> In currently code, nothing happens even if FORCE option is specified. >> This option completely exist for backward compatibility. >> But this patch add new syntax including FORCE option for now. > > > I forgot that. There's no reason to support it with the new stuff then. > > -- > Jim Nasby, Data Architect, Blue Treble Consulting > Data in Trouble? Get it in Treble! http://BlueTreble.com Regards, ------- Sawada Masahiko
On Tue, Mar 10, 2015 at 5:05 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 3/9/15 9:43 PM, Sawada Masahiko wrote: >> >> On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim.Nasby@bluetreble.com> >> wrote: >>> >>> On 3/2/15 10:58 AM, Sawada Masahiko wrote: >>>> >>>> >>>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim.Nasby@bluetreble.com> >>>> wrote: >>>>> >>>>> >>>>> On 2/24/15 8:28 AM, Sawada Masahiko wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> According to the above discussion, VACUUM and REINDEX should have >>>>>>> trailing options. Tom seems (to me) suggesting that SQL-style >>>>>>> (bare word preceded by WITH) options and Jim suggesting '()' >>>>>>> style options? (Anyway VACUUM gets the*third additional* option >>>>>>> sytle, but it is the different discussion from this) >>>>> >>>>> >>>>> >>>>> >>>>> Well, almost everything does a trailing WITH. We need to either stick >>>>> with >>>>> that for consistency, or add leading () as an option to those WITH >>>>> commands. >>>>> >>>>> Does anyone know why those are WITH? Is it ANSI? >>>>> >>>>> As a refresher, current commands are: >>>>> >>>>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>>>> REINDEX INDEX index1 FORCE; >>>>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>>>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH >>>>> DATA; >>>>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>>>> CREATE ROLE role WITH LOGIN; >>>>> GRANT .... WITH GRANT OPTION; >>>>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>>>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>>>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; >>> >>> >>> >>> BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the >>> most >>> consistent with everything else. Is there a problem with doing that? I >>> know >>> getting syntax is one of the hard parts of new features, but it seems >>> like >>> we reached consensus here... >> >> >> Attached is latest version patch based on Tom's idea as follows. >> REINDEX { INDEX | ... } name WITH ( options [, ...] ) > > > Are the parenthesis necessary? No other WITH option requires them, other > than create table/matview (COPY doesn't actually require them). > >>>> We have discussed about this option including FORCE option, but I >>>> think there are not user who want to use both FORCE and VERBOSE option >>>> at same time. >>> >>> >>> >>> I find that very hard to believe... I would expect a primary use case for >>> VERBOSE to be "I ran REINDEX, but it doesn't seem to have done >>> anything... >>> what's going on?" and that's exactly when you might want to use FORCE. >>> >> >> In currently code, nothing happens even if FORCE option is specified. >> This option completely exist for backward compatibility. >> But this patch add new syntax including FORCE option for now. > > > I forgot that. There's no reason to support it with the new stuff then. > > -- > Jim Nasby, Data Architect, Blue Treble Consulting > Data in Trouble? Get it in Treble! http://BlueTreble.com Attached patch is latest version patch changed syntax a little. This patch adds following syntax for now. REINDEX { INDEX | ... } name WITH (VERBOSE); But we are under the discussion regarding parenthesis, so there is possibility of change. Regards, ------- Sawada Masahiko
Вложения
On 3/11/15 6:33 AM, Sawada Masahiko wrote: >>>>>> As a refresher, current commands are: >>>>>> >>>>> >>>>>> >>>>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>>>>> >>>>> REINDEX INDEX index1 FORCE; >>>>>> >>>>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>>>>> >>>>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry WITH >>>>>> >>>>>DATA; >>>>>> >>>>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>>>>> >>>>> CREATE ROLE role WITH LOGIN; >>>>>> >>>>> GRANT .... WITH GRANT OPTION; >>>>>> >>>>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>>>>> >>>>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>>>>> >>>>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; >>>> >>> >>>> >>> >>>> >>> >>>> >>>BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be the >>>> >>>most >>>> >>>consistent with everything else. Is there a problem with doing that? I >>>> >>>know >>>> >>>getting syntax is one of the hard parts of new features, but it seems >>>> >>>like >>>> >>>we reached consensus here... >>> >> >>> >> >>> >>Attached is latest version patch based on Tom's idea as follows. >>> >>REINDEX { INDEX | ... } name WITH ( options [, ...] ) >> > >> > >> >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. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > On 3/11/15 6:33 AM, Sawada Masahiko wrote: >>>>>>> >>>>>>> As a refresher, current commands are: >>>>>>> >>>>> >>>>>>> >>>>> VACUUM (ANALYZE, VERBOSE) table1 (col1); >>>>>>> >>>>> REINDEX INDEX index1 FORCE; >>>>>>> >>>>> COPY table1 FROM 'file.txt' WITH (FORMAT csv); >>>>>>> >>>>> CREATE MATERIALIZED VIEW mv1 WITH (storageparam, ...) AS qry >>>>>>> >>>>> WITH >>>>>>> >>>>>DATA; >>>>>>> >>>>> CREATE EXTENSION ext1 WITH SCHEMA s1 VERSION v1 FROM over; >>>>>>> >>>>> CREATE ROLE role WITH LOGIN; >>>>>>> >>>>> GRANT .... WITH GRANT OPTION; >>>>>>> >>>>> CREATE VIEW v1 AS qry WITH CASCADED CHECK OPTION; >>>>>>> >>>>> ALTER DATABASE db1 WITH CONNECTION LIMIT 50; >>>>>>> >>>>> DECLARE c1 INSENSITIVE SCROLL CURSOR WITH HOLD; >>>>> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>>BTW, I'm fine with Tom's bare-word with WITH idea. That seems to be >>>>> >>> the >>>>> >>>most >>>>> >>>consistent with everything else. Is there a problem with doing that? >>>>> >>> I >>>>> >>>know >>>>> >>>getting syntax is one of the hard parts of new features, but it >>>>> >>> seems >>>>> >>>like >>>>> >>>we reached consensus here... >>>> >>>> >> >>>> >> >>>> >>Attached is latest version patch based on Tom's idea as follows. >>>> >>REINDEX { INDEX | ... } name WITH ( options [, ...] ) >>> >>> > >>> > >>> >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. Regards, ------- Sawada Masahiko
Вложения
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. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On Wed, Mar 11, 2015 at 5:36 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > 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. Generally, I think the commands that don't have () are the older ones, and those that do have it are the newer ones: EXPLAIN, VERBOSE, the newest of our three COPY syntaxes, CREATE MATERIALIZED VIEW, foreign data wrappers, servers, and foreign tables. The older stuff like CREATE DATABASE and REINDEX that uses ad-hoc syntax instead is a real pain in the neck: every time you want to add an option, you've got to add new parser rules and keywords, which is bad for the overall efficiency of parsing. So I think this argument is exactly backwards: parenthesized options are the newer, better way to do this sort of thing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 3/13/15 6:48 AM, Robert Haas wrote: > On Wed, Mar 11, 2015 at 5:36 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: >> 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. > > Generally, I think the commands that don't have () are the older ones, > and those that do have it are the newer ones: EXPLAIN, VERBOSE, the > newest of our three COPY syntaxes, CREATE MATERIALIZED VIEW, foreign > data wrappers, servers, and foreign tables. The older stuff like > CREATE DATABASE and REINDEX that uses ad-hoc syntax instead is a real > pain in the neck: every time you want to add an option, you've got to > add new parser rules and keywords, which is bad for the overall > efficiency of parsing. So I think this argument is exactly backwards: > parenthesized options are the newer, better way to do this sort of > thing. Yeah, that doesn't sound like a good tradeoff compared to making people type some extra ()s. :( We should at least support ()s on the other commands though, so that we're consistent. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.com
On Fri, Mar 13, 2015 at 8:57 AM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote: > Yeah, that doesn't sound like a good tradeoff compared to making people type > some extra ()s. :( > > We should at least support ()s on the other commands though, so that we're > consistent. I think we've been moving slowly in that direction, but it's not this patch's job to accelerate that transition. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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
Вложения
<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>
On Tue, Apr 7, 2015 at 9:32 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada.mshk@gmail.com> > wrote: >> >> 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. >> > > 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, > > Thank you for your reviewing. I modified the patch and attached latest version patch(v7). Please have a look it. Regards, ------- Sawada Masahiko
Вложения
On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> Thank you for your reviewing.
> I modified the patch and attached latest version patch(v7).
> Please have a look it.
>
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
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
Вложения
On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> > wrote: >> >> Thank you for your reviewing. >> I modified the patch and attached latest version patch(v7). >> Please have a look it. >> > > Looks good to me. Attached patch (v8) just fix a tab indentation in gram.y. > I had forgotten fix a tab indentation, sorry. Thank you for reviewing! It looks good to me too. Can this patch be marked as "Ready for Committer"? Regards, ------- Sawada Masahiko
<div dir="ltr"><div class="gmail_extra"><br />On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>>wrote:<br />><br />> On Tue, Apr 7, 2015 at 10:12PM, Fabrízio de Royes Mello<br />> <<a href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>> wrote:<br/>> ><br />> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>><br/>> > wrote:<br />> >><br />> >>Thank you for your reviewing.<br />> >> I modified the patch and attached latest version patch(v7).<br />>>> Please have a look it.<br />> >><br />> ><br />> > Looks good to me. Attached patch (v8)just fix a tab indentation in gram.y.<br />> ><br />><br />> I had forgotten fix a tab indentation, sorry.<br/>> Thank you for reviewing!<br />> It looks good to me too.<br />> Can this patch be marked as "Readyfor Committer"?<br />><br /><br />+1<br /><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br/>>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog: <ahref="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>
On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> > wrote: >> >> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >> <fabriziomello@gmail.com> wrote: >> > >> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >> > wrote: >> >> >> >> Thank you for your reviewing. >> >> I modified the patch and attached latest version patch(v7). >> >> Please have a look it. >> >> >> > >> > Looks good to me. Attached patch (v8) just fix a tab indentation in >> > gram.y. >> > >> >> I had forgotten fix a tab indentation, sorry. >> Thank you for reviewing! >> It looks good to me too. >> Can this patch be marked as "Ready for Committer"? >> > > +1 > Changed status to "Ready for Committer". Thank you for final reviewing! Regards, ------- Sawada Masahiko
On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> >> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> >> wrote: >>> >>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >>> <fabriziomello@gmail.com> wrote: >>> > >>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >>> > wrote: >>> >> >>> >> Thank you for your reviewing. >>> >> I modified the patch and attached latest version patch(v7). >>> >> Please have a look it. >>> >> >>> > >>> > Looks good to me. Attached patch (v8) just fix a tab indentation in >>> > gram.y. >>> > >>> >>> I had forgotten fix a tab indentation, sorry. >>> Thank you for reviewing! >>> It looks good to me too. >>> Can this patch be marked as "Ready for Committer"? >>> >> >> +1 >> > > Changed status to "Ready for Committer". The patch adds new syntax like "REINDEX ... WITH VERBOSE", i.e., () is not added after WITH clause. Did we reach the consensus about this syntax? The last email from Robert just makes me think that () should be added into the syntax. Regards, -- Fujii Masao
On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello >> <fabriziomello@gmail.com> wrote: >>> >>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>> wrote: >>>> >>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >>>> <fabriziomello@gmail.com> wrote: >>>> > >>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >>>> > wrote: >>>> >> >>>> >> Thank you for your reviewing. >>>> >> I modified the patch and attached latest version patch(v7). >>>> >> Please have a look it. >>>> >> >>>> > >>>> > Looks good to me. Attached patch (v8) just fix a tab indentation in >>>> > gram.y. >>>> > >>>> >>>> I had forgotten fix a tab indentation, sorry. >>>> Thank you for reviewing! >>>> It looks good to me too. >>>> Can this patch be marked as "Ready for Committer"? >>>> >>> >>> +1 >>> >> >> Changed status to "Ready for Committer". > > The patch adds new syntax like "REINDEX ... WITH VERBOSE", i.e., () is not > added after WITH clause. Did we reach the consensus about this syntax? > The last email from Robert just makes me think that () should be added > into the syntax. > Thank you for taking time for this patch! This was quite complicated issue since we already have a lot of style command currently. We can think many of style from various perspective: kind of DDL, new or old command, maintenance command. And each command has each its story. I believe we have reached the consensus with this style at least once (please see previous discussion), but we might needs to discuss more... Regards, ------- Sawada Masahiko
On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello >>> <fabriziomello@gmail.com> wrote: >>>> >>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>>> wrote: >>>>> >>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >>>>> <fabriziomello@gmail.com> wrote: >>>>> > >>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >>>>> > wrote: >>>>> >> >>>>> >> Thank you for your reviewing. >>>>> >> I modified the patch and attached latest version patch(v7). >>>>> >> Please have a look it. >>>>> >> >>>>> > >>>>> > Looks good to me. Attached patch (v8) just fix a tab indentation in >>>>> > gram.y. >>>>> > >>>>> >>>>> I had forgotten fix a tab indentation, sorry. >>>>> Thank you for reviewing! >>>>> It looks good to me too. >>>>> Can this patch be marked as "Ready for Committer"? >>>>> >>>> >>>> +1 >>>> >>> >>> Changed status to "Ready for Committer". >> >> The patch adds new syntax like "REINDEX ... WITH VERBOSE", i.e., () is not >> added after WITH clause. Did we reach the consensus about this syntax? >> The last email from Robert just makes me think that () should be added >> into the syntax. >> > > Thank you for taking time for this patch! I removed the FORCE option from REINDEX, so you would need to update the patch. > This was quite complicated issue since we already have a lot of style > command currently. > We can think many of style from various perspective: kind of DDL, new > or old command, maintenance command. And each command has each its > story. > I believe we have reached the consensus with this style at least once > (please see previous discussion), but we might needs to discuss > more... Okay, another question is that; WITH must be required whenever the options are specified? Or should it be abbreviatable? Regards, -- Fujii Masao
On Thu, Apr 9, 2015 at 1:14 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello >>>> <fabriziomello@gmail.com> wrote: >>>>> >>>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>>>> wrote: >>>>>> >>>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >>>>>> <fabriziomello@gmail.com> wrote: >>>>>> > >>>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >>>>>> > wrote: >>>>>> >> >>>>>> >> Thank you for your reviewing. >>>>>> >> I modified the patch and attached latest version patch(v7). >>>>>> >> Please have a look it. >>>>>> >> >>>>>> > >>>>>> > Looks good to me. Attached patch (v8) just fix a tab indentation in >>>>>> > gram.y. >>>>>> > >>>>>> >>>>>> I had forgotten fix a tab indentation, sorry. >>>>>> Thank you for reviewing! >>>>>> It looks good to me too. >>>>>> Can this patch be marked as "Ready for Committer"? >>>>>> >>>>> >>>>> +1 >>>>> >>>> >>>> Changed status to "Ready for Committer". >>> >>> The patch adds new syntax like "REINDEX ... WITH VERBOSE", i.e., () is not >>> added after WITH clause. Did we reach the consensus about this syntax? >>> The last email from Robert just makes me think that () should be added >>> into the syntax. >>> >> >> Thank you for taking time for this patch! > > I removed the FORCE option from REINDEX, so you would need to update the patch. Thanks. I will change the patch with this change. >> This was quite complicated issue since we already have a lot of style >> command currently. >> We can think many of style from various perspective: kind of DDL, new >> or old command, maintenance command. And each command has each its >> story. >> I believe we have reached the consensus with this style at least once >> (please see previous discussion), but we might needs to discuss >> more... > > Okay, another question is that; WITH must be required whenever the options > are specified? Or should it be abbreviatable? In previous discussion, the WITH clause is always required by VERBOSE option. I don't think to enable us to abbreviate WITH clause for now. Also, at this time that FORCE option is removed, we could bring back idea is to put VERBOSE at after object name like CLUSTER. (INDEX, TABLE, etc.) Regards, ------- Sawada Masahiko
On Fri, Apr 10, 2015 at 2:52 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, Apr 9, 2015 at 1:14 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>>> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>>>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello >>>>> <fabriziomello@gmail.com> wrote: >>>>>> >>>>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello >>>>>>> <fabriziomello@gmail.com> wrote: >>>>>>> > >>>>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada.mshk@gmail.com> >>>>>>> > wrote: >>>>>>> >> >>>>>>> >> Thank you for your reviewing. >>>>>>> >> I modified the patch and attached latest version patch(v7). >>>>>>> >> Please have a look it. >>>>>>> >> >>>>>>> > >>>>>>> > Looks good to me. Attached patch (v8) just fix a tab indentation in >>>>>>> > gram.y. >>>>>>> > >>>>>>> >>>>>>> I had forgotten fix a tab indentation, sorry. >>>>>>> Thank you for reviewing! >>>>>>> It looks good to me too. >>>>>>> Can this patch be marked as "Ready for Committer"? >>>>>>> >>>>>> >>>>>> +1 >>>>>> >>>>> >>>>> Changed status to "Ready for Committer". >>>> >>>> The patch adds new syntax like "REINDEX ... WITH VERBOSE", i.e., () is not >>>> added after WITH clause. Did we reach the consensus about this syntax? >>>> The last email from Robert just makes me think that () should be added >>>> into the syntax. >>>> >>> >>> Thank you for taking time for this patch! >> >> I removed the FORCE option from REINDEX, so you would need to update the patch. > > Thanks. > I will change the patch with this change. > >>> This was quite complicated issue since we already have a lot of style >>> command currently. >>> We can think many of style from various perspective: kind of DDL, new >>> or old command, maintenance command. And each command has each its >>> story. >>> I believe we have reached the consensus with this style at least once >>> (please see previous discussion), but we might needs to discuss >>> more... >> >> Okay, another question is that; WITH must be required whenever the options >> are specified? Or should it be abbreviatable? > > In previous discussion, the WITH clause is always required by VERBOSE > option. I don't think to enable us to abbreviate WITH clause for now. > Also, at this time that FORCE option is removed, we could bring back > idea is to put VERBOSE at after object name like CLUSTER. (INDEX, > TABLE, etc.) > Attached v10 patch is latest version patch. The syntax is, REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ] That is, WITH clause is optional. Regards, ------- Sawada Masahiko
Вложения
On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > Attached v10 patch is latest version patch. > The syntax is, > REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ] > > That is, WITH clause is optional. I thought we agreed on moving this earlier in the command: http://www.postgresql.org/message-id/18569.1423159294@sss.pgh.pa.us -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> Attached v10 patch is latest version patch. >> The syntax is, >> REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ] >> >> That is, WITH clause is optional. > > I thought we agreed on moving this earlier in the command: > > http://www.postgresql.org/message-id/18569.1423159294@sss.pgh.pa.us > Oh, I see. Attached patch is modified syntax as REINDEX [VERBOSE] { INDEX | ... } name Thought? Regards, ------- Sawada Masahiko
Вложения
On Thu, Apr 30, 2015 at 10:15 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> >> Attached v10 patch is latest version patch.
> >> The syntax is,
> >> REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ]
> >>
> >> That is, WITH clause is optional.
> >
> > I thought we agreed on moving this earlier in the command:
> >
> > http://www.postgresql.org/message-id/18569.1423159294@sss.pgh.pa.us
> >
>
> Oh, I see.
> Attached patch is modified syntax as
> REINDEX [VERBOSE] { INDEX | ... } name
>
> Thought?
>
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
On Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> Attached v10 patch is latest version patch. >>> The syntax is, >>> REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ] >>> >>> That is, WITH clause is optional. >> >> I thought we agreed on moving this earlier in the command: >> >> http://www.postgresql.org/message-id/18569.1423159294@sss.pgh.pa.us >> > > Oh, I see. > Attached patch is modified syntax as > REINDEX [VERBOSE] { INDEX | ... } name > > Thought? I thought what we agreed on was: REINDEX (flexible options) { INDEX | ... } name The argument wasn't about whether to use flexible options, but where in the command to put them. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, May 1, 2015 at 1:38 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>>> Attached v10 patch is latest version patch. >>>> The syntax is, >>>> REINDEX { INDEX | ... } name [ WITH ] [ VERBOSE ] >>>> >>>> That is, WITH clause is optional. >>> >>> I thought we agreed on moving this earlier in the command: >>> >>> http://www.postgresql.org/message-id/18569.1423159294@sss.pgh.pa.us >>> >> >> Oh, I see. >> Attached patch is modified syntax as >> REINDEX [VERBOSE] { INDEX | ... } name >> >> Thought? > > I thought what we agreed on was: > > REINDEX (flexible options) { INDEX | ... } name > > The argument wasn't about whether to use flexible options, but where > in the command to put them. > VACUUM has both syntax: with parentheses and without parentheses. I think we should have both syntax for REINDEX like VACUUM does because it would be pain to put parentheses whenever we want to do REINDEX. Are the parentheses optional in REINDEX command? And CLUSTER should have syntax like that in future? Regards, ------- Sawada Masahiko
On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > VACUUM has both syntax: with parentheses and without parentheses. > I think we should have both syntax for REINDEX like VACUUM does > because it would be pain to put parentheses whenever we want to do > REINDEX. > Are the parentheses optional in REINDEX command? No. The unparenthesized VACUUM syntax was added back before we realized that that kind of syntax is a terrible idea. It requires every option to be a keyword, and those keywords have to be in a fixed order. I believe the intention is to keep the old VACUUM syntax around for backward-compatibility, but not to extend it. Same for EXPLAIN and COPY. I agree that it would be nice if the grammar problems could be solved without adding parentheses. But there was a period during which many good ideas for new EXPLAIN options died on the vine because we were using an inextensible syntax for EXPLAIN options. I'm not keen to repeat that experience. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> VACUUM has both syntax: with parentheses and without parentheses. >> I think we should have both syntax for REINDEX like VACUUM does >> because it would be pain to put parentheses whenever we want to do >> REINDEX. >> Are the parentheses optional in REINDEX command? > > No. The unparenthesized VACUUM syntax was added back before we > realized that that kind of syntax is a terrible idea. It requires > every option to be a keyword, and those keywords have to be in a fixed > order. I believe the intention is to keep the old VACUUM syntax > around for backward-compatibility, but not to extend it. Same for > EXPLAIN and COPY. REINDEX will have only one option VERBOSE for now. Even we're in a situation like that it's not clear to be added newly additional option to REINDEX now, we should need to put parenthesis? Also I'm not sure that both implementation and documentation regarding VERBOSE option should be optional. Regards, ------- Sawada Masahiko
On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> VACUUM has both syntax: with parentheses and without parentheses. >>> I think we should have both syntax for REINDEX like VACUUM does >>> because it would be pain to put parentheses whenever we want to do >>> REINDEX. >>> Are the parentheses optional in REINDEX command? >> >> No. The unparenthesized VACUUM syntax was added back before we >> realized that that kind of syntax is a terrible idea. It requires >> every option to be a keyword, and those keywords have to be in a fixed >> order. I believe the intention is to keep the old VACUUM syntax >> around for backward-compatibility, but not to extend it. Same for >> EXPLAIN and COPY. > > REINDEX will have only one option VERBOSE for now. > Even we're in a situation like that it's not clear to be added newly > additional option to REINDEX now, we should need to put parenthesis? In my opinion, yes. The whole point of a flexible options syntax is that we can add new options without changing the grammar. That involves some compromise on the syntax, which doesn't bother me a bit. Our previous experiments with this for EXPLAIN and COPY and VACUUM have worked out quite well, and I see no reason for pessimism here. > Also I'm not sure that both implementation and documentation regarding > VERBOSE option should be optional. I don't know what this means. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>>>> VACUUM has both syntax: with parentheses and without parentheses.
>>>> I think we should have both syntax for REINDEX like VACUUM does
>>>> because it would be pain to put parentheses whenever we want to do
>>>> REINDEX.
>>>> Are the parentheses optional in REINDEX command?
>>>
>>> No. The unparenthesized VACUUM syntax was added back before we
>>> realized that that kind of syntax is a terrible idea. It requires
>>> every option to be a keyword, and those keywords have to be in a fixed
>>> order. I believe the intention is to keep the old VACUUM syntax
>>> around for backward-compatibility, but not to extend it. Same for
>>> EXPLAIN and COPY.
>>
>> REINDEX will have only one option VERBOSE for now.
>> Even we're in a situation like that it's not clear to be added newly
>> additional option to REINDEX now, we should need to put parenthesis?
>
> In my opinion, yes. The whole point of a flexible options syntax is
> that we can add new options without changing the grammar. That
> involves some compromise on the syntax, which doesn't bother me a bit.
> Our previous experiments with this for EXPLAIN and COPY and VACUUM
> have worked out quite well, and I see no reason for pessimism here.
I agree that flexible option syntax does not need to change grammar whenever we add new options.
Attached patch is changed based on your suggestion.
And the patch for reindexdb is also attached.
Please feedbacks.
>> Also I'm not sure that both implementation and documentation regarding
>> VERBOSE option should be optional.
>
> I don't know what this means.
>
Sorry for confusing you.
Regards,
-------
Sawada Masahiko
Please feedbacks.
>> Also I'm not sure that both implementation and documentation regarding
>> VERBOSE option should be optional.
>
> I don't know what this means.
>
Sorry for confusing you.
Please ignore this.
Regards,
-------
Sawada Masahiko
--
Regards,
-------
Sawada Masahiko
On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com > <javascript:;>> wrote: >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada.mshk@gmail.com > <javascript:;>> wrote: >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com > <javascript:;>> wrote: >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko >>>> <sawada.mshk@gmail.com > <javascript:;>> wrote: >>>>> VACUUM has both syntax: with parentheses and without parentheses. >>>>> I think we should have both syntax for REINDEX like VACUUM does >>>>> because it would be pain to put parentheses whenever we want to do >>>>> REINDEX. >>>>> Are the parentheses optional in REINDEX command? >>>> >>>> No. The unparenthesized VACUUM syntax was added back before we >>>> realized that that kind of syntax is a terrible idea. It requires >>>> every option to be a keyword, and those keywords have to be in a fixed >>>> order. I believe the intention is to keep the old VACUUM syntax >>>> around for backward-compatibility, but not to extend it. Same for >>>> EXPLAIN and COPY. >>> >>> REINDEX will have only one option VERBOSE for now. >>> Even we're in a situation like that it's not clear to be added newly >>> additional option to REINDEX now, we should need to put parenthesis? >> >> In my opinion, yes. The whole point of a flexible options syntax is >> that we can add new options without changing the grammar. That >> involves some compromise on the syntax, which doesn't bother me a bit. >> Our previous experiments with this for EXPLAIN and COPY and VACUUM >> have worked out quite well, and I see no reason for pessimism here. > > I agree that flexible option syntax does not need to change grammar > whenever we add new options. > Attached patch is changed based on your suggestion. > And the patch for reindexdb is also attached. > Please feedbacks. > >>> Also I'm not sure that both implementation and documentation regarding >>> VERBOSE option should be optional. >> >> I don't know what this means. >> > > Sorry for confusing you. > Please ignore this. > Sorry, I forgot attach files. Regards, ------- Sawada Masahiko
Вложения
On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
>
> On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com
> > <javascript:;>> wrote:
> >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada.mshk@gmail.com
> > <javascript:;>> wrote:
> >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com
> > <javascript:;>> wrote:
> >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
> >>>> <sawada.mshk@gmail.com
> > <javascript:;>> wrote:
> >>>>> VACUUM has both syntax: with parentheses and without parentheses.
> >>>>> I think we should have both syntax for REINDEX like VACUUM does
> >>>>> because it would be pain to put parentheses whenever we want to do
> >>>>> REINDEX.
> >>>>> Are the parentheses optional in REINDEX command?
> >>>>
> >>>> No. The unparenthesized VACUUM syntax was added back before we
> >>>> realized that that kind of syntax is a terrible idea. It requires
> >>>> every option to be a keyword, and those keywords have to be in a fixed
> >>>> order. I believe the intention is to keep the old VACUUM syntax
> >>>> around for backward-compatibility, but not to extend it. Same for
> >>>> EXPLAIN and COPY.
> >>>
> >>> REINDEX will have only one option VERBOSE for now.
> >>> Even we're in a situation like that it's not clear to be added newly
> >>> additional option to REINDEX now, we should need to put parenthesis?
> >>
> >> In my opinion, yes. The whole point of a flexible options syntax is
> >> that we can add new options without changing the grammar. That
> >> involves some compromise on the syntax, which doesn't bother me a bit.
> >> Our previous experiments with this for EXPLAIN and COPY and VACUUM
> >> have worked out quite well, and I see no reason for pessimism here.
> >
> > I agree that flexible option syntax does not need to change grammar
> > whenever we add new options.
> > Attached patch is changed based on your suggestion.
> > And the patch for reindexdb is also attached.
> > Please feedbacks.
> >
> >>> Also I'm not sure that both implementation and documentation regarding
> >>> VERBOSE option should be optional.
> >>
> >> I don't know what this means.
> >>
> >
> > Sorry for confusing you.
> > Please ignore this.
> >
>
> Sorry, I forgot attach files.
>
tab-complete.c: In function ‘psql_completion’:
tab-complete.c:3338:12: warning: left-hand operand of comma expression has no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:21: warning: left-hand operand of comma expression has no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:31: warning: left-hand operand of comma expression has no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:41: warning: left-hand operand of comma expression has no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:53: warning: left-hand operand of comma expression has no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
^
tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in this function)
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
tab-complete.c:3340:22: note: each undeclared identifier is reported only once for each function it appears in
COMPLETE_WITH_LIST(list_REINDEX);
^
tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
completion_charpp = list; \
^
make[3]: *** [tab-complete.o] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [install-psql-recurse] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [install-bin-recurse] Error 2
make: *** [install-src-recurse] Error 2
Looking at the code I think you remove one line accidentally from tab-complete.c:
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int end)
/* REINDEX */
else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
{
- static const char *const list_REINDEX[] =
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
COMPLETE_WITH_LIST(list_REINDEX);
$ git diff src/bin/psql/tab-complete.c
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 750e29d..55b0df5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int end)
/* REINDEX */
else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
{
- static const char *const list_REINDEX[] =
{"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
COMPLETE_WITH_LIST(list_REINDEX);
The attached fix it and now seems good to me.
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
Вложения
On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote:
>
>
> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> >
> > On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote:
> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com
> > > <javascript:;>> wrote:
> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada.mshk@gmail.com
> > > <javascript:;>> wrote:
> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com
> > > <javascript:;>> wrote:
> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
> > >>>> <sawada.mshk@gmail.com
> > > <javascript:;>> wrote:
> > >>>>> VACUUM has both syntax: with parentheses and without parentheses.
> > >>>>> I think we should have both syntax for REINDEX like VACUUM does
> > >>>>> because it would be pain to put parentheses whenever we want to do
> > >>>>> REINDEX.
> > >>>>> Are the parentheses optional in REINDEX command?
> > >>>>
> > >>>> No. The unparenthesized VACUUM syntax was added back before we
> > >>>> realized that that kind of syntax is a terrible idea. It requires
> > >>>> every option to be a keyword, and those keywords have to be in a fixed
> > >>>> order. I believe the intention is to keep the old VACUUM syntax
> > >>>> around for backward-compatibility, but not to extend it. Same for
> > >>>> EXPLAIN and COPY.
> > >>>
> > >>> REINDEX will have only one option VERBOSE for now.
> > >>> Even we're in a situation like that it's not clear to be added newly
> > >>> additional option to REINDEX now, we should need to put parenthesis?
> > >>
> > >> In my opinion, yes. The whole point of a flexible options syntax is
> > >> that we can add new options without changing the grammar. That
> > >> involves some compromise on the syntax, which doesn't bother me a bit.
> > >> Our previous experiments with this for EXPLAIN and COPY and VACUUM
> > >> have worked out quite well, and I see no reason for pessimism here.
> > >
> > > I agree that flexible option syntax does not need to change grammar
> > > whenever we add new options.
> > > Attached patch is changed based on your suggestion.
> > > And the patch for reindexdb is also attached.
> > > Please feedbacks.
> > >
> > >>> Also I'm not sure that both implementation and documentation regarding
> > >>> VERBOSE option should be optional.
> > >>
> > >> I don't know what this means.
> > >>
> > >
> > > Sorry for confusing you.
> > > Please ignore this.
> > >
> >
> > Sorry, I forgot attach files.
> >
>
> I applied the two patches to master and I got some errors when compile:
>
> tab-complete.c: In function ‘psql_completion’:
> tab-complete.c:3338:12: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:21: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:31: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:41: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:53: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value]
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
> ^
> tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in this function)
> COMPLETE_WITH_LIST(list_REINDEX);
> ^
> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
> completion_charpp = list; \
> ^
> tab-complete.c:3340:22: note: each undeclared identifier is reported only once for each function it appears in
> COMPLETE_WITH_LIST(list_REINDEX);
> ^
> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’
> completion_charpp = list; \
> ^
> make[3]: *** [tab-complete.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [install-psql-recurse] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [install-bin-recurse] Error 2
> make: *** [install-src-recurse] Error 2
>
>
> Looking at the code I think you remove one line accidentally from tab-complete.c:
>
> $ git diff src/bin/psql/tab-complete.c
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 750e29d..55b0df5 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int end)
> /* REINDEX */
> else if (pg_strcasecmp(prev_wd, "REINDEX") == 0)
> {
> - static const char *const list_REINDEX[] =
> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL};
>
> COMPLETE_WITH_LIST(list_REINDEX);
>
>
> The attached fix it and now seems good to me.
>
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
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
On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > > On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> >> >> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> >> wrote: >> > >> > On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com >> > > <javascript:;>> wrote: >> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko >> > >> <sawada.mshk@gmail.com >> > > <javascript:;>> wrote: >> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com >> > > <javascript:;>> wrote: >> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko >> > >>>> <sawada.mshk@gmail.com >> > > <javascript:;>> wrote: >> > >>>>> VACUUM has both syntax: with parentheses and without parentheses. >> > >>>>> I think we should have both syntax for REINDEX like VACUUM does >> > >>>>> because it would be pain to put parentheses whenever we want to do >> > >>>>> REINDEX. >> > >>>>> Are the parentheses optional in REINDEX command? >> > >>>> >> > >>>> No. The unparenthesized VACUUM syntax was added back before we >> > >>>> realized that that kind of syntax is a terrible idea. It requires >> > >>>> every option to be a keyword, and those keywords have to be in a >> > >>>> fixed >> > >>>> order. I believe the intention is to keep the old VACUUM syntax >> > >>>> around for backward-compatibility, but not to extend it. Same for >> > >>>> EXPLAIN and COPY. >> > >>> >> > >>> REINDEX will have only one option VERBOSE for now. >> > >>> Even we're in a situation like that it's not clear to be added newly >> > >>> additional option to REINDEX now, we should need to put parenthesis? >> > >> >> > >> In my opinion, yes. The whole point of a flexible options syntax is >> > >> that we can add new options without changing the grammar. That >> > >> involves some compromise on the syntax, which doesn't bother me a >> > >> bit. >> > >> Our previous experiments with this for EXPLAIN and COPY and VACUUM >> > >> have worked out quite well, and I see no reason for pessimism here. >> > > >> > > I agree that flexible option syntax does not need to change grammar >> > > whenever we add new options. >> > > Attached patch is changed based on your suggestion. >> > > And the patch for reindexdb is also attached. >> > > Please feedbacks. >> > > >> > >>> Also I'm not sure that both implementation and documentation >> > >>> regarding >> > >>> VERBOSE option should be optional. >> > >> >> > >> I don't know what this means. >> > >> >> > > >> > > Sorry for confusing you. >> > > Please ignore this. >> > > >> > >> > Sorry, I forgot attach files. >> > >> >> I applied the two patches to master and I got some errors when compile: >> >> tab-complete.c: In function ‘psql_completion’: >> tab-complete.c:3338:12: warning: left-hand operand of comma expression has >> no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:21: warning: left-hand operand of comma expression has >> no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:31: warning: left-hand operand of comma expression has >> no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:41: warning: left-hand operand of comma expression has >> no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:53: warning: left-hand operand of comma expression has >> no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value] >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> ^ >> tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in >> this function) >> COMPLETE_WITH_LIST(list_REINDEX); >> ^ >> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >> completion_charpp = list; \ >> ^ >> tab-complete.c:3340:22: note: each undeclared identifier is reported only >> once for each function it appears in >> COMPLETE_WITH_LIST(list_REINDEX); >> ^ >> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >> completion_charpp = list; \ >> ^ >> make[3]: *** [tab-complete.o] Error 1 >> make[3]: *** Waiting for unfinished jobs.... >> make[2]: *** [install-psql-recurse] Error 2 >> make[2]: *** Waiting for unfinished jobs.... >> make[1]: *** [install-bin-recurse] Error 2 >> make: *** [install-src-recurse] Error 2 >> >> >> Looking at the code I think you remove one line accidentally from >> tab-complete.c: >> >> $ git diff src/bin/psql/tab-complete.c >> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c >> index 750e29d..55b0df5 100644 >> --- a/src/bin/psql/tab-complete.c >> +++ b/src/bin/psql/tab-complete.c >> @@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int >> end) >> /* REINDEX */ >> else if (pg_strcasecmp(prev_wd, "REINDEX") == 0) >> { >> - static const char *const list_REINDEX[] = >> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >> >> COMPLETE_WITH_LIST(list_REINDEX); >> >> >> The attached fix it and now seems good to me. > Just one last note. IMHO we should add a regression to > src/bin/scripts/090_reindexdb.pl. > Thank you for your patch! (Sorry for attaching the patch still has compile error..) - 000_reindex_verbose_v13.patch Looks good to me. - 001_reindexdb_verbose_option_v1.patch I noticed a bug in reindexdb patch, so fixed version is attached. The regression test for reindexdb is added as well. Regards, ------- Sawada Masahiko
Вложения
On Sun, May 10, 2015 at 2:23 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> >> >> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello >> <fabriziomello@gmail.com> wrote: >>> >>> >>> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>> wrote: >>> > >>> > On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com >>> > > <javascript:;>> wrote: >>> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko >>> > >> <sawada.mshk@gmail.com >>> > > <javascript:;>> wrote: >>> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com >>> > > <javascript:;>> wrote: >>> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko >>> > >>>> <sawada.mshk@gmail.com >>> > > <javascript:;>> wrote: >>> > >>>>> VACUUM has both syntax: with parentheses and without parentheses. >>> > >>>>> I think we should have both syntax for REINDEX like VACUUM does >>> > >>>>> because it would be pain to put parentheses whenever we want to do >>> > >>>>> REINDEX. >>> > >>>>> Are the parentheses optional in REINDEX command? >>> > >>>> >>> > >>>> No. The unparenthesized VACUUM syntax was added back before we >>> > >>>> realized that that kind of syntax is a terrible idea. It requires >>> > >>>> every option to be a keyword, and those keywords have to be in a >>> > >>>> fixed >>> > >>>> order. I believe the intention is to keep the old VACUUM syntax >>> > >>>> around for backward-compatibility, but not to extend it. Same for >>> > >>>> EXPLAIN and COPY. >>> > >>> >>> > >>> REINDEX will have only one option VERBOSE for now. >>> > >>> Even we're in a situation like that it's not clear to be added newly >>> > >>> additional option to REINDEX now, we should need to put parenthesis? >>> > >> >>> > >> In my opinion, yes. The whole point of a flexible options syntax is >>> > >> that we can add new options without changing the grammar. That >>> > >> involves some compromise on the syntax, which doesn't bother me a >>> > >> bit. >>> > >> Our previous experiments with this for EXPLAIN and COPY and VACUUM >>> > >> have worked out quite well, and I see no reason for pessimism here. >>> > > >>> > > I agree that flexible option syntax does not need to change grammar >>> > > whenever we add new options. >>> > > Attached patch is changed based on your suggestion. >>> > > And the patch for reindexdb is also attached. >>> > > Please feedbacks. >>> > > >>> > >>> Also I'm not sure that both implementation and documentation >>> > >>> regarding >>> > >>> VERBOSE option should be optional. >>> > >> >>> > >> I don't know what this means. >>> > >> >>> > > >>> > > Sorry for confusing you. >>> > > Please ignore this. >>> > > >>> > >>> > Sorry, I forgot attach files. >>> > >>> >>> I applied the two patches to master and I got some errors when compile: >>> >>> tab-complete.c: In function ‘psql_completion’: >>> tab-complete.c:3338:12: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:21: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:31: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:41: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:53: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in >>> this function) >>> COMPLETE_WITH_LIST(list_REINDEX); >>> ^ >>> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >>> completion_charpp = list; \ >>> ^ >>> tab-complete.c:3340:22: note: each undeclared identifier is reported only >>> once for each function it appears in >>> COMPLETE_WITH_LIST(list_REINDEX); >>> ^ >>> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >>> completion_charpp = list; \ >>> ^ >>> make[3]: *** [tab-complete.o] Error 1 >>> make[3]: *** Waiting for unfinished jobs.... >>> make[2]: *** [install-psql-recurse] Error 2 >>> make[2]: *** Waiting for unfinished jobs.... >>> make[1]: *** [install-bin-recurse] Error 2 >>> make: *** [install-src-recurse] Error 2 >>> >>> >>> Looking at the code I think you remove one line accidentally from >>> tab-complete.c: >>> >>> $ git diff src/bin/psql/tab-complete.c >>> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c >>> index 750e29d..55b0df5 100644 >>> --- a/src/bin/psql/tab-complete.c >>> +++ b/src/bin/psql/tab-complete.c >>> @@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int >>> end) >>> /* REINDEX */ >>> else if (pg_strcasecmp(prev_wd, "REINDEX") == 0) >>> { >>> - static const char *const list_REINDEX[] = >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> >>> COMPLETE_WITH_LIST(list_REINDEX); >>> >>> >>> The attached fix it and now seems good to me. >> Just one last note. IMHO we should add a regression to >> src/bin/scripts/090_reindexdb.pl. >> > > Thank you for your patch! > (Sorry for attaching the patch still has compile error..) > > - 000_reindex_verbose_v13.patch > Looks good to me. > > - 001_reindexdb_verbose_option_v1.patch > I noticed a bug in reindexdb patch, so fixed version is attached. > The regression test for reindexdb is added as well. I forgot to add documentation patch. The latest version patch is attached. (the latest version of REINDEX VERBOSE patch is v13 Fabrizio attached) Regards, ------- Sawada Masahiko
Вложения
On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > Sorry, I forgot attach files. Review comments: - Customarily we use int, rather than uint8, for flags variables. I think we should do that here also. - reindex_index() emits a log message either way, but at DEBUG2 level without VERBOSE and at the INFO level with it. I think we should skip it altogether without VERBOSE. i.e. if (options & REINDEXOPT_VERBOSE) ereport(...) - The errmsg() in that function should not end with a period. - The 000 patch makes a pointless whitespace change to tab-complete.c. - Instead of an enumerated type (ReindexOption) just use #define to define the flag value. Apart from those fairly minor issues, I think this looks pretty solid. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, May 14, 2015 at 12:28 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> Sorry, I forgot attach files. > > Review comments: > > - Customarily we use int, rather than uint8, for flags variables. I > think we should do that here also. > > - reindex_index() emits a log message either way, but at DEBUG2 level > without VERBOSE and at the INFO level with it. I think we should skip > it altogether without VERBOSE. i.e. if (options & REINDEXOPT_VERBOSE) > ereport(...) > > - The errmsg() in that function should not end with a period. > > - The 000 patch makes a pointless whitespace change to tab-complete.c. > > - Instead of an enumerated type (ReindexOption) just use #define to > define the flag value. > > Apart from those fairly minor issues, I think this looks pretty solid. > Thank you for reviwing.. All fixed. Regards, ------- Sawada Masahiko
Вложения
Uh, are we really using INFO to log this? I thought that was a discouraged level to use anymore. Why not NOTICE? Also, when multiple tables are reindexed, do we emit lines for each index, or only for each table? If we're going to emit a line for each index in the single-table mode, it seems more sensible to do the same for the multi-table forms also. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, May 14, 2015 at 3:10 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Uh, are we really using INFO to log this? I thought that was a > discouraged level to use anymore. Why not NOTICE? > I think it should be INFO level because it is a information of REINDEX command,such as progress of itself, CPU usage and so on. it would be overkill if we output the logs as NOTICE level, and verbose outputs of other maintenance command are emitted as INFO level. > Also, when multiple tables are reindexed, do we emit lines for each > index, or only for each table? If we're going to emit a line for each > index in the single-table mode, it seems more sensible to do the same > for the multi-table forms also. > I agree that we emit lines for each table when we do reindex multiple tables. The latest patch is attached. Regards, ------- Sawada Masahiko
Вложения
On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Uh, are we really using INFO to log this? I thought that was a > discouraged level to use anymore. Why not NOTICE? Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any good reason to make this different. > Also, when multiple tables are reindexed, do we emit lines for each > index, or only for each table? If we're going to emit a line for each > index in the single-table mode, it seems more sensible to do the same > for the multi-table forms also. Hmm, yeah, I agree with that. I thought the patch worked that way, but I see now that it doesn't. I think that should be changed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas wrote: > On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Uh, are we really using INFO to log this? I thought that was a > > discouraged level to use anymore. Why not NOTICE? > > Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any > good reason to make this different. I was misremembering the INFO situation. Here's one item in the archives I found in a very quick search, which says that INFO is the right thing to use: http://www.postgresql.org/message-id/24874.1231183906@sss.pgh.pa.us Cheers -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, May 14, 2015 at 4:53 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Uh, are we really using INFO to log this? I thought that was a >> discouraged level to use anymore. Why not NOTICE? > > Well, as Masahiko-san points out, VACUUM uses INFO. I can't see any > good reason to make this different. > >> Also, when multiple tables are reindexed, do we emit lines for each >> index, or only for each table? If we're going to emit a line for each >> index in the single-table mode, it seems more sensible to do the same >> for the multi-table forms also. > > Hmm, yeah, I agree with that. I thought the patch worked that way, > but I see now that it doesn't. I think that should be changed. > The v15 patch emits a line for each table when reindexing multiple tables, and emits a line for each index when reindexing single table. But v14 patch emits a line for each index, regardless of reindex target. Should I change back to v14 patch? Regards, ------- Sawada Masahiko
<div dir="ltr"><div class="gmail_extra"><br />On Wed, May 13, 2015 at 2:49 PM, Sawada Masahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>>wrote:<br />><br />> On Thu, May 14, 2015 at 12:28AM, Robert Haas <<a href="mailto:robertmhaas@gmail.com">robertmhaas@gmail.com</a>> wrote:<br />> > On Thu,May 7, 2015 at 6:55 PM, Sawada Masahiko <<a href="mailto:sawada.mshk@gmail.com">sawada.mshk@gmail.com</a>> wrote:<br/>> >> Sorry, I forgot attach files.<br />> ><br />> > Review comments:<br />> ><br />>> - Customarily we use int, rather than uint8, for flags variables. I<br />> > think we should do that herealso.<br />> ><br />> > - reindex_index() emits a log message either way, but at DEBUG2 level<br />> >without VERBOSE and at the INFO level with it. I think we should skip<br />> > it altogether without VERBOSE. i.e. if (options & REINDEXOPT_VERBOSE)<br />> > ereport(...)<br />> ><br />> > - The errmsg()in that function should not end with a period.<br />> ><br />> > - The 000 patch makes a pointless whitespacechange to tab-complete.c.<br />> ><br />> > - Instead of an enumerated type (ReindexOption) just use#define to<br />> > define the flag value.<br />> ><br />> > Apart from those fairly minor issues, Ithink this looks pretty solid.<br />> ><br />><br />> Thank you for reviwing..<br />> All fixed.<br />><br/><br /></div><div class="gmail_extra">IMHO we don't need "pg_rusage_init(&ru0)" if the verbose options is notsetted. Maybe change:<br /><br />+<br />+ pg_rusage_init(&ru0);<br /><br /></div><div class="gmail_extra">to<br/><br />+<br />+ if (options & REINDEXOPT_VERBOSE)<br />+ pg_rusage_init(&ru0);<br/></div><div class="gmail_extra"><br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><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>
On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > The v15 patch emits a line for each table when reindexing multiple > tables, and emits a line for each index when reindexing single table. > But v14 patch emits a line for each index, regardless of reindex target. > Should I change back to v14 patch? Uh, maybe. What made you change it? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, May 14, 2015 at 9:58 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >> The v15 patch emits a line for each table when reindexing multiple >> tables, and emits a line for each index when reindexing single table. >> But v14 patch emits a line for each index, regardless of reindex target. >> Should I change back to v14 patch? > > Uh, maybe. What made you change it? > I thought that the users who want to reindex multiple tables are interested in the time to reindex whole table takes. But I think it seems sensible to emit a line for each index even when reindex multiple tables. The v16 patch is based on v14 and a few modified is attached. Regards, ------- Sawada Masahiko
Вложения
On Thu, May 14, 2015 at 4:30 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Thu, May 14, 2015 at 9:58 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> The v15 patch emits a line for each table when reindexing multiple >>> tables, and emits a line for each index when reindexing single table. >>> But v14 patch emits a line for each index, regardless of reindex target. >>> Should I change back to v14 patch? >> >> Uh, maybe. What made you change it? >> > > I thought that the users who want to reindex multiple tables are > interested in the time to reindex whole table takes. > But I think it seems sensible to emit a line for each index even when > reindex multiple tables. > The v16 patch is based on v14 and a few modified is attached. Thanks for updating the patch! The regression test failed because you forgot to remove the trailng period from the verbose message in the "expected file" of the regression test. I just fixed it and push the patch. Regards, -- Fujii Masao
On Sun, May 10, 2015 at 2:23 AM, Sawada Masahiko <sawada.mshk@gmail.com> wrote: > On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello > <fabriziomello@gmail.com> wrote: >> >> >> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello >> <fabriziomello@gmail.com> wrote: >>> >>> >>> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada.mshk@gmail.com> >>> wrote: >>> > >>> > On 5/7/15, Sawada Masahiko <sawada.mshk@gmail.com> wrote: >>> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas@gmail.com >>> > > <javascript:;>> wrote: >>> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko >>> > >> <sawada.mshk@gmail.com >>> > > <javascript:;>> wrote: >>> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas@gmail.com >>> > > <javascript:;>> wrote: >>> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko >>> > >>>> <sawada.mshk@gmail.com >>> > > <javascript:;>> wrote: >>> > >>>>> VACUUM has both syntax: with parentheses and without parentheses. >>> > >>>>> I think we should have both syntax for REINDEX like VACUUM does >>> > >>>>> because it would be pain to put parentheses whenever we want to do >>> > >>>>> REINDEX. >>> > >>>>> Are the parentheses optional in REINDEX command? >>> > >>>> >>> > >>>> No. The unparenthesized VACUUM syntax was added back before we >>> > >>>> realized that that kind of syntax is a terrible idea. It requires >>> > >>>> every option to be a keyword, and those keywords have to be in a >>> > >>>> fixed >>> > >>>> order. I believe the intention is to keep the old VACUUM syntax >>> > >>>> around for backward-compatibility, but not to extend it. Same for >>> > >>>> EXPLAIN and COPY. >>> > >>> >>> > >>> REINDEX will have only one option VERBOSE for now. >>> > >>> Even we're in a situation like that it's not clear to be added newly >>> > >>> additional option to REINDEX now, we should need to put parenthesis? >>> > >> >>> > >> In my opinion, yes. The whole point of a flexible options syntax is >>> > >> that we can add new options without changing the grammar. That >>> > >> involves some compromise on the syntax, which doesn't bother me a >>> > >> bit. >>> > >> Our previous experiments with this for EXPLAIN and COPY and VACUUM >>> > >> have worked out quite well, and I see no reason for pessimism here. >>> > > >>> > > I agree that flexible option syntax does not need to change grammar >>> > > whenever we add new options. >>> > > Attached patch is changed based on your suggestion. >>> > > And the patch for reindexdb is also attached. >>> > > Please feedbacks. >>> > > >>> > >>> Also I'm not sure that both implementation and documentation >>> > >>> regarding >>> > >>> VERBOSE option should be optional. >>> > >> >>> > >> I don't know what this means. >>> > >> >>> > > >>> > > Sorry for confusing you. >>> > > Please ignore this. >>> > > >>> > >>> > Sorry, I forgot attach files. >>> > >>> >>> I applied the two patches to master and I got some errors when compile: >>> >>> tab-complete.c: In function ‘psql_completion’: >>> tab-complete.c:3338:12: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:21: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:31: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:41: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:53: warning: left-hand operand of comma expression has >>> no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:5: warning: statement with no effect [-Wunused-value] >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3338:59: error: expected ‘;’ before ‘}’ token >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> ^ >>> tab-complete.c:3340:22: error: ‘list_REINDEX’ undeclared (first use in >>> this function) >>> COMPLETE_WITH_LIST(list_REINDEX); >>> ^ >>> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >>> completion_charpp = list; \ >>> ^ >>> tab-complete.c:3340:22: note: each undeclared identifier is reported only >>> once for each function it appears in >>> COMPLETE_WITH_LIST(list_REINDEX); >>> ^ >>> tab-complete.c:169:22: note: in definition of macro ‘COMPLETE_WITH_LIST’ >>> completion_charpp = list; \ >>> ^ >>> make[3]: *** [tab-complete.o] Error 1 >>> make[3]: *** Waiting for unfinished jobs.... >>> make[2]: *** [install-psql-recurse] Error 2 >>> make[2]: *** Waiting for unfinished jobs.... >>> make[1]: *** [install-bin-recurse] Error 2 >>> make: *** [install-src-recurse] Error 2 >>> >>> >>> Looking at the code I think you remove one line accidentally from >>> tab-complete.c: >>> >>> $ git diff src/bin/psql/tab-complete.c >>> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c >>> index 750e29d..55b0df5 100644 >>> --- a/src/bin/psql/tab-complete.c >>> +++ b/src/bin/psql/tab-complete.c >>> @@ -3335,7 +3335,6 @@ psql_completion(const char *text, int start, int >>> end) >>> /* REINDEX */ >>> else if (pg_strcasecmp(prev_wd, "REINDEX") == 0) >>> { >>> - static const char *const list_REINDEX[] = >>> {"TABLE", "INDEX", "SYSTEM", "SCHEMA", "DATABASE", NULL}; >>> >>> COMPLETE_WITH_LIST(list_REINDEX); >>> >>> >>> The attached fix it and now seems good to me. >> Just one last note. IMHO we should add a regression to >> src/bin/scripts/090_reindexdb.pl. >> > > Thank you for your patch! > (Sorry for attaching the patch still has compile error..) > > - 000_reindex_verbose_v13.patch > Looks good to me. > > - 001_reindexdb_verbose_option_v1.patch > I noticed a bug in reindexdb patch, so fixed version is attached. > The regression test for reindexdb is added as well. Pushed. Regards, -- Fujii Masao