Re: 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(dot)mshk(at)gmail(dot)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(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Feb 2, 2015 at 8:31 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> writes:
> On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> writes:
>> On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)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

On Tue, Feb 3, 2015 at 9:09 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> writes:
> >> On Mon, Feb 2, 2015 at 9:21 PM, Michael Paquier
> >> <michael(dot)paquier(at)gmail(dot)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?
>

I don't think the keyworks INDEX, TABLE, SCHEMA, SYSTEM and DATABASE are
options... they are part of the command IMHO.

Maybe something like:

REINDEX { INDEX | TABLE | SCHEMA | SYSTEM | DATABASE } [( [ FORCE ],
[VERBOSE] ) ] name;

And maintain the old syntax for compatibility of course.

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

Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> writes:
> On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(dot)mshk(at)gmail(dot)com> writes:
> > On Tue, Feb 3, 2015 at 12:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)2ndquadrant(dot)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(dot)Nasby(at)bluetreble(dot)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

On Tue, Feb 3, 2015 at 8:26 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
> 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).
>

Makes sense... +1

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 2/3/15 5:08 PM, Tom Lane wrote:
> Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)BlueTreble(dot)com> writes:
> On 2/3/15 5:08 PM, Tom Lane wrote:
>> Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jim Nasby <Jim(dot)Nasby(at)BlueTreble(dot)com> writes:
>> On 2/3/15 5:08 PM, Tom Lane wrote:
>>> Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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 &lt;

> horiguchi(dot)kyotaro(at)(dot)co

> &gt; 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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(at)gmail(dot)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(at)gmail(dot)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(dot)Nasby(at)BlueTreble(dot)com> wrote in <54DBBCC9(dot)1020206(at)BlueTreble(dot)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(dot)Nasby(at)BlueTreble(dot)com> wrote in<54DBBCC9(dot)1020206(at)BlueTreble(dot)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(dot)Nasby(at)BlueTreble(dot)com> wrote in <54DBBCC9(dot)1020206(at)BlueTreble(dot)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(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBkjndQAq2Z-WbScMdHoX257BJj6sUYthwwFs2iL8YDhQ(at)mail(dot)gmail(dot)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(dot)Nasby(at)BlueTreble(dot)com> wrote in <54E381AD(dot)1090409(at)BlueTreble(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(dot)Nasby(at)BlueTreble(dot)com> wrote in <54DBBCC9(dot)1020206(at)BlueTreble(dot)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(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBkjndQAq2Z-WbScMdHoX257BJj6sUYthwwFs2iL8YDhQ(at)mail(dot)gmail(dot)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);
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;
--
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(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 3/2/15 10:58 AM, Sawada Masahiko wrote:
>>
>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)com> wrote:
>> On 3/2/15 10:58 AM, Sawada Masahiko wrote:
>>>
>>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 3/9/15 9:43 PM, Sawada Masahiko wrote:
>>
>> On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
>> wrote:
>>>
>>> On 3/2/15 10:58 AM, Sawada Masahiko wrote:
>>>>
>>>>
>>>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 3/9/15 9:43 PM, Sawada Masahiko wrote:
>>
>> On Fri, Mar 6, 2015 at 11:07 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
>> wrote:
>>>
>>> On 3/2/15 10:58 AM, Sawada Masahiko wrote:
>>>>
>>>>
>>>> On Wed, Feb 25, 2015 at 4:58 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)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(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBxPCpPvKQmvJMUh+p=2pfAu03gKJQ2R2zY47XHsH205Q(at)mail(dot)gmail(dot)com>
sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> >>> >Are the parenthesis necessary? No other WITH option requires them, other
> >>> >than create table/matview (COPY doesn't actually require them).
> >>> >
> >>
> >> I was imagining EXPLAIN syntax.
> >> Is there some possibility of supporting multiple options for REINDEX
> >> command in future?
> >> If there is, syntax will be as follows, REINDEX { INDEX | ... } name
> >> WITH VERBOSE, XXX, XXX;
> >> I thought style with parenthesis is better than above style.
> >
> >
> > The thing is, ()s are actually an odd-duck. Very little supports it, and
> > while COPY allows it they're not required. EXPLAIN is a different story,
> > because that's not WITH; we're actually using () *instead of* WITH.
> >
> > So because almost all commands that use WITH doen't even accept (), I don't
> > think this should either. It certainly shouldn't require them, because
> > unlike EXPLAIN, there's no need to require them.
> >
>
> I understood what your point is.
> Attached patch is changed syntax, it does not have parenthesis.

As I looked into the code to find what the syntax would be, I
found some points which would be better to be fixed.

In gram.y the options is a list of cstring but it is not necesary
to be a list because there's only one kind of option now.

If you prefer it to be a list, I have a comment for the way to
make string list in gram.y. You stored bare cstring in the
options list but I think it is not the preferable form. I suppose
the followings are preferable. Corresponding fixes are needed in
ReindexTable, ReindexIndex, ReindexMultipleTables.

$$ = list_make1(makeString($1);
....
$$ = lappend($1, list_make1(makeString($3));

In equalfuncs.c, _equalReindexStmt forgets to compare the member
options. _copyReindexStmt also forgets to copy it. The way you
constructed the options list prevents them from doing their jobs
using prepared methods. Comparing and copying the member "option"
is needed even if it becomes a simple string.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Mar 11, 2015 at 5:36 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)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(dot)Nasby(at)bluetreble(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello, I have some trivial comments about the latest patch.
>
> At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBxPCpPvKQmvJMUh+p=2pfAu03gKJQ2R2zY47XHsH205Q(at)mail(dot)gmail(dot)com>
> sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> >>> >Are the parenthesis necessary? No other WITH option requires them, other
>> >>> >than create table/matview (COPY doesn't actually require them).
>> >>> >
>> >>
>> >> I was imagining EXPLAIN syntax.
>> >> Is there some possibility of supporting multiple options for REINDEX
>> >> command in future?
>> >> If there is, syntax will be as follows, REINDEX { INDEX | ... } name
>> >> WITH VERBOSE, XXX, XXX;
>> >> I thought style with parenthesis is better than above style.
>> >
>> >
>> > The thing is, ()s are actually an odd-duck. Very little supports it, and
>> > while COPY allows it they're not required. EXPLAIN is a different story,
>> > because that's not WITH; we're actually using () *instead of* WITH.
>> >
>> > So because almost all commands that use WITH doen't even accept (), I don't
>> > think this should either. It certainly shouldn't require them, because
>> > unlike EXPLAIN, there's no need to require them.
>> >
>>
>> I understood what your point is.
>> Attached patch is changed syntax, it does not have parenthesis.
>
> As I looked into the code to find what the syntax would be, I
> found some points which would be better to be fixed.
>
> In gram.y the options is a list of cstring but it is not necesary
> to be a list because there's only one kind of option now.
>
> If you prefer it to be a list, I have a comment for the way to
> make string list in gram.y. You stored bare cstring in the
> options list but I think it is not the preferable form. I suppose
> the followings are preferable. Corresponding fixes are needed in
> ReindexTable, ReindexIndex, ReindexMultipleTables.
>
> $$ = list_make1(makeString($1);
> ....
> $$ = lappend($1, list_make1(makeString($3));
>
>
> In equalfuncs.c, _equalReindexStmt forgets to compare the member
> options. _copyReindexStmt also forgets to copy it. The way you
> constructed the options list prevents them from doing their jobs
> using prepared methods. Comparing and copying the member "option"
> is needed even if it becomes a simple string.
>

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

On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > Hello, I have some trivial comments about the latest patch.
> >
> > At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko <
sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoBxPCpPvKQmvJMUh+p=
2pfAu03gKJQ2R2zY47XHsH205Q(at)mail(dot)gmail(dot)com>
> > sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby <
Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> >> >>> >Are the parenthesis necessary? No other WITH option requires
them, other
> >> >>> >than create table/matview (COPY doesn't actually require them).
> >> >>> >
> >> >>
> >> >> I was imagining EXPLAIN syntax.
> >> >> Is there some possibility of supporting multiple options for REINDEX
> >> >> command in future?
> >> >> If there is, syntax will be as follows, REINDEX { INDEX | ... } name
> >> >> WITH VERBOSE, XXX, XXX;
> >> >> I thought style with parenthesis is better than above style.
> >> >
> >> >
> >> > The thing is, ()s are actually an odd-duck. Very little supports it,
and
> >> > while COPY allows it they're not required. EXPLAIN is a different
story,
> >> > because that's not WITH; we're actually using () *instead of* WITH.
> >> >
> >> > So because almost all commands that use WITH doen't even accept (),
I don't
> >> > think this should either. It certainly shouldn't require them,
because
> >> > unlike EXPLAIN, there's no need to require them.
> >> >
> >>
> >> I understood what your point is.
> >> Attached patch is changed syntax, it does not have parenthesis.
> >
> > As I looked into the code to find what the syntax would be, I
> > found some points which would be better to be fixed.
> >
> > In gram.y the options is a list of cstring but it is not necesary
> > to be a list because there's only one kind of option now.
> >
> > If you prefer it to be a list, I have a comment for the way to
> > make string list in gram.y. You stored bare cstring in the
> > options list but I think it is not the preferable form. I suppose
> > the followings are preferable. Corresponding fixes are needed in
> > ReindexTable, ReindexIndex, ReindexMultipleTables.
> >
> > $ = list_make1(makeString($1);
> > ....
> > $ = lappend($1, list_make1(makeString($3));
> >
> >
> > In equalfuncs.c, _equalReindexStmt forgets to compare the member
> > options. _copyReindexStmt also forgets to copy it. The way you
> > constructed the options list prevents them from doing their jobs
> > using prepared methods. Comparing and copying the member "option"
> > is needed even if it becomes a simple string.
> >
>
> I revised patch, and changed gram.y as I don't use the list.
> So this patch adds new syntax,
> REINDEX { INDEX | ... } name WITH VERBOSE;
>
> Also documentation is updated.
> Please give me feedbacks.
>

Some notes:

1) There are a trailing space in src/backend/parser/gram.y:

- | REINDEX DATABASE name opt_force
+ | REINDEX reindex_target_multitable name WITH opt_verbose
{
ReindexStmt *n = makeNode(ReindexStmt);
- n->kind = REINDEX_OBJECT_DATABASE;
+ n->kind = $2;
n->name = $3;
n->relation = NULL;
+ n->verbose = $5;
$$ = (Node *)n;
}
;

2) The documentation was updated and is according the behaviour.

3) psql autocomplete is ok.

4) Lack of regression tests. I think you should add some regression like
that:

fabrizio=# \set VERBOSITY terse
fabrizio=# create table reindex_verbose(id integer primary key);
CREATE TABLE
fabrizio=# reindex table reindex_verbose with verbose;
INFO: index "reindex_verbose_pkey" was reindexed.
REINDEX

5) Code style and organization is ok

6) You should add the new field ReindexStmt->verbose to
src/backend/nodes/copyfuncs.c

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Tue, Apr 7, 2015 at 9:32 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
> On Mon, Apr 6, 2015 at 10:21 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
>>
>> On Fri, Mar 13, 2015 at 5:10 PM, Kyotaro HORIGUCHI
>> <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> > Hello, I have some trivial comments about the latest patch.
>> >
>> > At Thu, 12 Mar 2015 21:15:14 +0900, Sawada Masahiko
>> > <sawada(dot)mshk(at)gmail(dot)com> wrote in
>> > <CAD21AoBxPCpPvKQmvJMUh+p=2pfAu03gKJQ2R2zY47XHsH205Q(at)mail(dot)gmail(dot)com>
>> > sawada.mshk> On Thu, Mar 12, 2015 at 6:36 AM, Jim Nasby
>> > <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> >> >>> >Are the parenthesis necessary? No other WITH option requires them,
>> >> >>> > other
>> >> >>> >than create table/matview (COPY doesn't actually require them).
>> >> >>> >
>> >> >>
>> >> >> I was imagining EXPLAIN syntax.
>> >> >> Is there some possibility of supporting multiple options for REINDEX
>> >> >> command in future?
>> >> >> If there is, syntax will be as follows, REINDEX { INDEX | ... } name
>> >> >> WITH VERBOSE, XXX, XXX;
>> >> >> I thought style with parenthesis is better than above style.
>> >> >
>> >> >
>> >> > The thing is, ()s are actually an odd-duck. Very little supports it,
>> >> > and
>> >> > while COPY allows it they're not required. EXPLAIN is a different
>> >> > story,
>> >> > because that's not WITH; we're actually using () *instead of* WITH.
>> >> >
>> >> > So because almost all commands that use WITH doen't even accept (), I
>> >> > don't
>> >> > think this should either. It certainly shouldn't require them,
>> >> > because
>> >> > unlike EXPLAIN, there's no need to require them.
>> >> >
>> >>
>> >> I understood what your point is.
>> >> Attached patch is changed syntax, it does not have parenthesis.
>> >
>> > As I looked into the code to find what the syntax would be, I
>> > found some points which would be better to be fixed.
>> >
>> > In gram.y the options is a list of cstring but it is not necesary
>> > to be a list because there's only one kind of option now.
>> >
>> > If you prefer it to be a list, I have a comment for the way to
>> > make string list in gram.y. You stored bare cstring in the
>> > options list but I think it is not the preferable form. I suppose
>> > the followings are preferable. Corresponding fixes are needed in
>> > ReindexTable, ReindexIndex, ReindexMultipleTables.
>> >
>> > $ = list_make1(makeString($1);
>> > ....
>> > $ = lappend($1, list_make1(makeString($3));
>> >
>> >
>> > In equalfuncs.c, _equalReindexStmt forgets to compare the member
>> > options. _copyReindexStmt also forgets to copy it. The way you
>> > constructed the options list prevents them from doing their jobs
>> > using prepared methods. Comparing and copying the member "option"
>> > is needed even if it becomes a simple string.
>> >
>>
>> 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(dot)mshk(at)gmail(dot)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.

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 Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
> On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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

On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> >
> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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

--
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 Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
>>
>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>> >
>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>> wrote:
>>>
>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>>> <fabriziomello(at)gmail(dot)com> wrote:
>>> >
>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>>
>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>> wrote:
>>>>
>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>> >
>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>
>>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>>> wrote:
>>>>>
>>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>> >
>>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>>
>>>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>>>> wrote:
>>>>>>
>>>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>>>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>>> >
>>>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Apr 9, 2015 at 1:14 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Apr 8, 2015 at 10:53 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> On Wed, Apr 8, 2015 at 1:09 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> On Wed, Apr 8, 2015 at 1:57 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>>>> On Wed, Apr 8, 2015 at 1:11 AM, Fabrízio de Royes Mello
>>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>>>
>>>>>> On Tue, Apr 7, 2015 at 1:04 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Tue, Apr 7, 2015 at 10:12 PM, Fabrízio de Royes Mello
>>>>>>> <fabriziomello(at)gmail(dot)com> wrote:
>>>>>>> >
>>>>>>> > On Tue, Apr 7, 2015 at 7:22 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> > On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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?
>

Looks 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 Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, Apr 30, 2015 at 9:15 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> On Thu, Apr 30, 2015 at 8:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Apr 30, 2015 at 6:37 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(at)gmail(dot)com
<javascript:;>> wrote:
> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com
<javascript:;>> wrote:
>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
<javascript:;>> wrote:
>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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.

Regards,

-------
Sawada Masahiko

--
Regards,

-------
Sawada Masahiko

On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
> <javascript:;>> wrote:
>> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com
> <javascript:;>> wrote:
>>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> <javascript:;>> wrote:
>>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
>>>> <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com>
wrote:
>
> On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
> > <javascript:;>> wrote:
> >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com
> > <javascript:;>> wrote:
> >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> > <javascript:;>> wrote:
> >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
> >>>> <sawada(dot)mshk(at)gmail(dot)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

On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)com> wrote:
>
>
> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
> >
> > On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
> > > <javascript:;>> wrote:
> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko <
sawada(dot)mshk(at)gmail(dot)com
> > > <javascript:;>> wrote:
> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> > > <javascript:;>> wrote:
> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
> > >>>> <sawada(dot)mshk(at)gmail(dot)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.

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 Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
>
> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>>
>> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>> wrote:
>> >
>> > On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
>> > > <javascript:;>> wrote:
>> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
>> > >> <sawada(dot)mshk(at)gmail(dot)com
>> > > <javascript:;>> wrote:
>> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
>> > > <javascript:;>> wrote:
>> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
>> > >>>> <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>>
>> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>>
>>>
>>> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>> wrote:
>>> >
>>> > On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
>>> > >> <sawada(dot)mshk(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
>>> > >>>> <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)gmail(dot)com> wrote:
> On Wed, May 13, 2015 at 2:10 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)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

On Wed, May 13, 2015 at 2:49 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
wrote:
>
> On Thu, May 14, 2015 at 12:28 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
> > On Thu, May 7, 2015 at 6:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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.
>

IMHO we don't need "pg_rusage_init(&ru0)" if the verbose options is not
setted. Maybe change:

+
+ pg_rusage_init(&ru0);

to

+
+ if (options & REINDEXOPT_VERBOSE)
+ pg_rusage_init(&ru0);

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 Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(at)gmail(dot)com> wrote:
> On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Thu, May 14, 2015 at 9:58 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Wed, May 13, 2015 at 8:25 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)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(dot)mshk(at)gmail(dot)com> wrote:
> On Sat, May 9, 2015 at 4:26 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
>>
>>
>> On Fri, May 8, 2015 at 4:23 PM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)com> wrote:
>>>
>>>
>>> On Thu, May 7, 2015 at 7:55 PM, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com>
>>> wrote:
>>> >
>>> > On 5/7/15, Sawada Masahiko <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> > > On Wed, May 6, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >> On Tue, May 5, 2015 at 11:10 AM, Sawada Masahiko
>>> > >> <sawada(dot)mshk(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >>> On Fri, May 1, 2015 at 9:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com
>>> > > <javascript:;>> wrote:
>>> > >>>> On Thu, Apr 30, 2015 at 11:05 PM, Sawada Masahiko
>>> > >>>> <sawada(dot)mshk(at)gmail(dot)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