Обсуждение: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

Hi,

Attached patches are following:

* tab_completion_alter_index_set_statistics.patch
     - Add column name completion after ALTER COLUMN
     - Avoid schema completion after SET STATISTICS

* fix_manual_of_alter_index.patch
     - ALTER INDEX ALTER COLUMN syntax is able to use not only
       column number but also column name. So, I fixed the manual.

* tab_completion_alter_table_set_statistics.patch
     - Avoid schema completion after SET STATISTICS


Regards,
Tatsuro Yamada

Hi,

On 2018/11/26 11:05, Tatsuro Yamada wrote:
> Hi,
> 
> Attached patches are following:
> 
> * tab_completion_alter_index_set_statistics.patch
>      - Add column name completion after ALTER COLUMN
>      - Avoid schema completion after SET STATISTICS
> 
> * fix_manual_of_alter_index.patch
>      - ALTER INDEX ALTER COLUMN syntax is able to use not only
>        column number but also column name. So, I fixed the manual.
> 
> * tab_completion_alter_table_set_statistics.patch
>      - Avoid schema completion after SET STATISTICS

I couldn't write patches details on previous email, so I write
more explanation for that on this email.


* tab_completion_alter_index_set_statistics.patch
=======
There are two problems. You can use these DDL before testing.
   #create table hoge (a integer, b integer);
   #create index ind_hoge on hoge (a, (a + b), (a * b));

   1) Can't get column names

     # alter index ind_hoge alter column <tab!><tab!>... but can't complete.

   2) I expected column names for column numbers after "SET STATISTICS", but
      tab-completion gave schema names

     # alter index ind_hoge alter column expr SET STATISTICS <tab!>
     information_schema.  pg_catalog.          pg_temp_1.           pg_toast.            pg_toast_temp_1.     public.

Applied the patch:

   1) We can get column names after "alter column".

     # alter index ind_hoge alter column <tab!>
     a      expr   expr1

   2) Fix!
     # alter index ind_hoge alter column expr SET STATISTICS <tab!>
=======


* fix_manual_of_alter_index_v2.patch (Attached on this email)
=======
https://www.postgresql.org/docs/devel/sql-alterindex.html

The patch fixes the syntax on above manual.
   s/column_number/column_number \| column_name/
And also it adds an explanation for column_name.

Syntax of ALTER INDEX ALTER COLUMN SET STATISTICS on the manual is below:

   ALTER INDEX [ IF EXISTS ] name ALTER [ COLUMN ] column_number
       SET STATISTICS integer

But we can use not only column number but also column name like this:

   # alter index ind_hoge alter column 2 SET STATISTICS 100;
   ALTER INDEX
   # alter index ind_hoge alter column expr SET STATISTICS 100;
   ALTER INDEX
=======


* tab_completion_alter_table_set_statistics.patch
=======
For now, we can get schema name by tab-completion. It is not appropriate.

   # alter table hoge alter column a set statistics <tab!>
   information_schema.  pg_catalog.          pg_temp_1.           pg_toast.            pg_toast_temp_1.     public.

Applied the patch:

   # alter table hoge alter column a set statistics <tab!>
=======

Any feedback is welcome. :)

Thanks,
Tatsuro Yamada
NTT Open Source Software Center

Hello.

At Wed, 28 Nov 2018 11:27:23 +0900, Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp> wrote in
<d677594b-101a-6236-7774-94a7c1a7b56b@lab.ntt.co.jp>
> Hi,
> 
> On 2018/11/26 11:05, Tatsuro Yamada wrote:
> I couldn't write patches details on previous email, so I write
> more explanation for that on this email.
> 
> 
> * tab_completion_alter_index_set_statistics.patch
> =======
> There are two problems. You can use these DDL before testing.
>   #create table hoge (a integer, b integer);
>   #create index ind_hoge on hoge (a, (a + b), (a * b));
> 
>   1) Can't get column names
> 
>     # alter index ind_hoge alter column <tab!><tab!>... but can't complete.

Currently the only continueable rule to the rule is SET
STATISTICS so we usually expect the number of an expression
column there. Even though we actually name every expression
column in an index, users hardly see the names. The names are in
the index column number order in your example, but what if the
name of the first column were 'foo'?

=# alter index ind_hoge2 alter column 
expr   expr1  foo    

We could still *guess* what is expr or exrp1 but I don't think it
helps much. (Note: foo is not usable in this context as it's a
non-expression column.)

>   2) I expected column names for column numbers after "SET STATISTICS",
>   but
>      tab-completion gave schema names
> 
>     # alter index ind_hoge alter column expr SET STATISTICS <tab!>
>     information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
>     pg_toast_temp_1.  public.

This is the result of STATISTICS <things> completion. SET
STATISTICS always doesn't take statistics name so this is safe.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2018/11/28 13:14, Kyotaro HORIGUCHI wrote:
> Hello.
> 
> At Wed, 28 Nov 2018 11:27:23 +0900, Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp> wrote in
<d677594b-101a-6236-7774-94a7c1a7b56b@lab.ntt.co.jp>
>> Hi,
>>
>> On 2018/11/26 11:05, Tatsuro Yamada wrote:
>> I couldn't write patches details on previous email, so I write
>> more explanation for that on this email.
>>
>>
>> * tab_completion_alter_index_set_statistics.patch
>> =======
>> There are two problems. You can use these DDL before testing.
>>    #create table hoge (a integer, b integer);
>>    #create index ind_hoge on hoge (a, (a + b), (a * b));
>>
>>    1) Can't get column names
>>
>>      # alter index ind_hoge alter column <tab!><tab!>... but can't complete.
> 
> Currently the only continueable rule to the rule is SET
> STATISTICS so we usually expect the number of an expression
> column there. Even though we actually name every expression
> column in an index, users hardly see the names. The names are in
> the index column number order in your example, but what if the
> name of the first column were 'foo'?
> 
> =# alter index ind_hoge2 alter column
> expr   expr1  foo
> 
> We could still *guess* what is expr or exrp1 but I don't think it
> helps much. (Note: foo is not usable in this context as it's a
> non-expression column.)

Thanks for your comment.
We can get column name by using "\d index_name" like this:

# \d ind_hoge
        Index "public.ind_hoge"
  Column |  Type   | Key? | Definition
--------+---------+------+------------
  a      | integer | yes  | a
  expr   | integer | yes  | (a + b)
  expr1  | integer | yes  | (a * b)
btree, for table "public.hoge"

So, I suppose that it's easy to understand what column is an expression column.
Of course, user will get syntax error if user chose "a" column like a "foo" which is
non-expression column as you mentioned.
Probably, I will be able to fix the patch to get only expression columns from the index.
Should I do that?


Other example, if user wants to use column number, I suppose that user have to check a
definition of index and count the number of columns.

====
# create table hoge2(a integer, b integer, foo integer);
CREATE TABLE

# create index ind_hoge2 on hoge2((a+b), foo, (a*b));
CREATE INDEX
[local] postgres@postgres:9912=# \d ind_hoge2
        Index "public.ind_hoge2"
  Column |  Type   | Key? | Definition
--------+---------+------+------------
  expr   | integer | yes  | (a + b)
  foo    | integer | yes  | foo
  expr1  | integer | yes  | (a * b)
btree, for table "public.hoge2"

# alter index ind_hoge2 alter column 1 set statistics 1;
ALTER INDEX

# alter index ind_hoge2 alter column 2 set statistics 1;
ERROR:  cannot alter statistics on non-expression column "foo" of index "ind_hoge2"

# alter index ind_hoge2 alter column 3 set statistics 1;
ALTER INDEX
====

I prefer to use column name instead column number because
there is no column number on \d index_name and \d+ index_name.



>>    2) I expected column names for column numbers after "SET STATISTICS",
>>    but
>>       tab-completion gave schema names
>>
>>      # alter index ind_hoge alter column expr SET STATISTICS <tab!>
>>      information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
>>      pg_toast_temp_1.  public.
> 
> This is the result of STATISTICS <things> completion. SET
> STATISTICS always doesn't take statistics name so this is safe.

:)


Thanks,
Tatsuro Yamada
NTT Open Source Software Center

Hello.

At Wed, 28 Nov 2018 14:41:40 +0900, Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp> wrote in
<54bd214b-d0d3-8654-e71f-45e7b4f979f0@lab.ntt.co.jp>
> On 2018/11/28 13:14, Kyotaro HORIGUCHI wrote:
> > Hello.
> > At Wed, 28 Nov 2018 11:27:23 +0900, Tatsuro Yamada
> > <yamada.tatsuro@lab.ntt.co.jp> wrote in
> > <d677594b-101a-6236-7774-94a7c1a7b56b@lab.ntt.co.jp>
> >> Hi,
> >>
> >> On 2018/11/26 11:05, Tatsuro Yamada wrote:
> >> I couldn't write patches details on previous email, so I write
> >> more explanation for that on this email.
> >>
> >>
> >> * tab_completion_alter_index_set_statistics.patch
> >> =======
> >> There are two problems. You can use these DDL before testing.
> >>    #create table hoge (a integer, b integer);
> >>    #create index ind_hoge on hoge (a, (a + b), (a * b));
> >>
> >>    1) Can't get column names
> >>
> >>      # alter index ind_hoge alter column <tab!><tab!>... but can't
> >>      # complete.
> > Currently the only continueable rule to the rule is SET
> > STATISTICS so we usually expect the number of an expression
> > column there. Even though we actually name every expression
> > column in an index, users hardly see the names. The names are in
> > the index column number order in your example, but what if the
> > name of the first column were 'foo'?
> > =# alter index ind_hoge2 alter column
> > expr   expr1  foo
> > We could still *guess* what is expr or exrp1 but I don't think it
> > helps much. (Note: foo is not usable in this context as it's a
> > non-expression column.)
> 
> Thanks for your comment.
> We can get column name by using "\d index_name" like this:
> 
> # \d ind_hoge
>        Index "public.ind_hoge"
>  Column |  Type   | Key? | Definition
> --------+---------+------+------------
>  a      | integer | yes  | a
>  expr   | integer | yes  | (a + b)
>  expr1  | integer | yes  | (a * b)
> btree, for table "public.hoge"
> 
> So, I suppose that it's easy to understand what column is an
> expression column.

Yeah, actually we can find them there, but I don't think it's
popular. I suppose that most of people are unconcious of the
names since they didn't named them.  Moreover what makes me
uneasy with this is that it doesn't suggest attribute numbers,
which are firstly expected there. But anyway it is impossible
with readline since suggestions are sorted as strings. ("1",
"11", "12", "2" order, I don't think indexes often have that many
columns, though.)

If \d <table> showed the names of index columns, the completion
would be more convinsing. But I'm not sure it is useful..

\d hoge
...
Indexes:
    "ind_hoge" btree (a, (a + b) as expr, (a * b) as expr1)

By the way, for index expressions consists of just one function,
suggestions looks much sainer.

> Of course, user will get syntax error if user chose "a" column like a
> "foo" which is
> non-expression column as you mentioned.
> Probably, I will be able to fix the patch to get only expression
> columns from the index.
> Should I do that?

Nope. That's just too-much. We are already showing unusable
suggestions in other places, for example, exiting tables for
CREATE TABLE. (It is useful for me as it suggests what should be
placed there.)

> Other example, if user wants to use column number, I suppose that user
> have to check a
> definition of index and count the number of columns.
> ====
> # create table hoge2(a integer, b integer, foo integer);
> CREATE TABLE
> 
> # create index ind_hoge2 on hoge2((a+b), foo, (a*b));
> CREATE INDEX
> [local] postgres@postgres:9912=# \d ind_hoge2
>        Index "public.ind_hoge2"
>  Column |  Type   | Key? | Definition
> --------+---------+------+------------
>  expr   | integer | yes  | (a + b)
>  foo    | integer | yes  | foo
>  expr1  | integer | yes  | (a * b)
> btree, for table "public.hoge2"
> 
> # alter index ind_hoge2 alter column 1 set statistics 1;
> ALTER INDEX
> 
> # alter index ind_hoge2 alter column 2 set statistics 1;
> ERROR: cannot alter statistics on non-expression column "foo" of index
> "ind_hoge2"
> 
> # alter index ind_hoge2 alter column 3 set statistics 1;
> ALTER INDEX
> ====
> 
> I prefer to use column name instead column number because
> there is no column number on \d index_name and \d+ index_name.

Some people prefer names even if they are implicitly
given. Others (including myself) prefer numbers.

> >>    2) I expected column names for column numbers after "SET STATISTICS",
> >>    but
> >>       tab-completion gave schema names
> >>
> >>      # alter index ind_hoge alter column expr SET STATISTICS <tab!>
> >>      information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
> >>      pg_toast_temp_1.  public.
> > This is the result of STATISTICS <things> completion. SET
> > STATISTICS always doesn't take statistics name so this is safe.
> 
> :)

So I think it is reasonable.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

> At Wed, 28 Nov 2018 14:41:40 +0900, Tatsuro Yamada <yamada.tatsuro@lab.ntt.co.jp> wrote in <54bd214b-d0d3-8654->>>>
>>>> * tab_completion_alter_index_set_statistics.patch
>>>> =======
>>>> There are two problems. You can use these DDL before testing.
>>>>     #create table hoge (a integer, b integer);
>>>>     #create index ind_hoge on hoge (a, (a + b), (a * b));
>>>>
>>>>     1) Can't get column names
>>>>
>>>>       # alter index ind_hoge alter column <tab!><tab!>... but can't
>>>>       # complete.
>>> Currently the only continueable rule to the rule is SET
>>> STATISTICS so we usually expect the number of an expression
>>> column there. Even though we actually name every expression
>>> column in an index, users hardly see the names. The names are in
>>> the index column number order in your example, but what if the
>>> name of the first column were 'foo'?
>>> =# alter index ind_hoge2 alter column
>>> expr   expr1  foo
>>> We could still *guess* what is expr or exrp1 but I don't think it
>>> helps much. (Note: foo is not usable in this context as it's a
>>> non-expression column.)
>>
>> Thanks for your comment.
>> We can get column name by using "\d index_name" like this:
>>
>> # \d ind_hoge
>>         Index "public.ind_hoge"
>>   Column |  Type   | Key? | Definition
>> --------+---------+------+------------
>>   a      | integer | yes  | a
>>   expr   | integer | yes  | (a + b)
>>   expr1  | integer | yes  | (a * b)
>> btree, for table "public.hoge"
>>
>> So, I suppose that it's easy to understand what column is an
>> expression column.
> 
> Yeah, actually we can find them there, but I don't think it's
> popular. I suppose that most of people are unconcious of the
> names since they didn't named them.  Moreover what makes me
> uneasy with this is that it doesn't suggest attribute numbers,
> which are firstly expected there. But anyway it is impossible
> with readline since suggestions are sorted as strings. ("1",
> "11", "12", "2" order, I don't think indexes often have that many
> columns, though.)

Okay, I understand.

  - For now, there is no column_number column on "\d index_name" result.
    So, if tab-completion suggested column_numbers, user can't match these easily.

  - Suggested column_name and column_number are sorted as a string by readline.
    These are an unnatural. But this is another topic on this thread.

    Example:
      # alter index ind_hoge alter column <tab!>
      c       expr    expr1   expr10  expr11  expr2   expr3   expr4   expr5   expr6   expr7   expr8   expr9


>> Of course, user will get syntax error if user chose "a" column like a
>> "foo" which is
>> non-expression column as you mentioned.
>> Probably, I will be able to fix the patch to get only expression
>> columns from the index.
>> Should I do that?
> 
> Nope. That's just too-much. We are already showing unusable
> suggestions in other places, for example, exiting tables for
> CREATE TABLE. (It is useful for me as it suggests what should be
> placed there.)

I see.


>> I prefer to use column name instead column number because
>> there is no column number on \d index_name and \d+ index_name.
> 
> Some people prefer names even if they are implicitly
> given. Others (including myself) prefer numbers.

So, we should better to vote to decide implementation of the tab-completion.

Which is better to use either column_names or column_numbers?
I'd like to hear opinions from others. :)





>>>>     2) I expected column names for column numbers after "SET STATISTICS",
>>>>     but
>>>>        tab-completion gave schema names
>>>>
>>>>       # alter index ind_hoge alter column expr SET STATISTICS <tab!>
>>>>       information_schema.  pg_catalog.  pg_temp_1.  pg_toast.
>>>>       pg_toast_temp_1.  public.
>>> This is the result of STATISTICS <things> completion. SET
>>> STATISTICS always doesn't take statistics name so this is safe.
>>
>> :)
> 
> So I think it is reasonable.

Thanks.


Regards,
Tatsuro Yamada

On Tue, Dec 04, 2018 at 02:31:51PM +0900, Tatsuro Yamada wrote:
>  - For now, there is no column_number column on "\d index_name" result.
>    So, if tab-completion suggested column_numbers, user can't match
>    these easily.

Well, it depends how many columns an index definition has.  If that's
only a few then it is not really a problem.  However I agree that we
could add that in the output of \d for indexes just before the
definition to help with an easy match.  The columns showing up are
relkind-dependent so that's not an issue.  This would create some noise
in some regression tests.

> So, we should better to vote to decide implementation of the tab-completion.
>
> Which is better to use either column_names or column_numbers?
> I'd like to hear opinions from others. :)

There has been a discussion in this area this week where the conclusion
is that we had better use column numbers rather than names arbitrarily
generated by the server for pg_dump:
https://postgr.es/m/CAARsnT3UQ4V=yDNW468w8RqHfYiY9mpn2r_c5UkBJ97NAApUEw@mail.gmail.com

So my take on the matter is to use column numbers, and show only those
with an expression as index attributes with no expressions cannot have
statistics.

Looking at the patches, fix_manual_of_alter_index.patch does not matter
much as the documentation of ALTER INDEX already mentions some
compatibilities with ALTER TABLE.

+       /* ALTER TABLE ALTER [COLUMN] <foo> SET STATISTICS */
+       else if (HeadMatches("ALTER", "TABLE") && TailMatches("SET", "STATISTICS")){
+               /* We don't complete after "SET STATISTICS" */
+       }
Okay, this one makes sense taken independently as the current completion
is confusing.  Could you also do the same for ALTER INDEX?
--
Michael

On 2018/12/19 14:27, Michael Paquier wrote:
> On Tue, Dec 04, 2018 at 02:31:51PM +0900, Tatsuro Yamada wrote:
>>   - For now, there is no column_number column on "\d index_name" result.
>>     So, if tab-completion suggested column_numbers, user can't match
>>     these easily.
> 
> Well, it depends how many columns an index definition has.  If that's
> only a few then it is not really a problem.  However I agree that we
> could add that in the output of \d for indexes just before the
> definition to help with an easy match.  The columns showing up are
> relkind-dependent so that's not an issue.  This would create some noise
> in some regression tests.

I see.
Hopefully, I'll create new patch for adding column_number to \d for indexes.


>> So, we should better to vote to decide implementation of the tab-completion.
>>
>> Which is better to use either column_names or column_numbers?
>> I'd like to hear opinions from others. :)
> 
> There has been a discussion in this area this week where the conclusion
> is that we had better use column numbers rather than names arbitrarily
> generated by the server for pg_dump:
> https://postgr.es/m/CAARsnT3UQ4V=yDNW468w8RqHfYiY9mpn2r_c5UkBJ97NAApUEw@mail.gmail.com
> 
> So my take on the matter is to use column numbers, and show only those
> with an expression as index attributes with no expressions cannot have
> statistics.

Agreed.
I'll revise the patch replaced column_name with column_number.


> Looking at the patches, fix_manual_of_alter_index.patch does not matter
> much as the documentation of ALTER INDEX already mentions some
> compatibilities with ALTER TABLE.

Hmm... I confused because these parameter are not same. Please see below:

====
   https://www.postgresql.org/docs/11/sql-altertable.html

     ALTER [ COLUMN ] column_name SET STATISTICS integer

   https://www.postgresql.org/docs/current/sql-alterindex.html

     ALTER [ COLUMN ] column_number SET STATISTICS integer
====

If we can use both parameter column_name and column_number, it would be better to
describe both on the documents.


> +       /* ALTER TABLE ALTER [COLUMN] <foo> SET STATISTICS */
> +       else if (HeadMatches("ALTER", "TABLE") && TailMatches("SET", "STATISTICS")){
> +               /* We don't complete after "SET STATISTICS" */
> +       }
> Okay, this one makes sense taken independently as the current completion
> is confusing.  Could you also do the same for ALTER INDEX?

Yep, I already did that for ALTER INDEX in tab_completion_alter_index_set_statistics.patch like this:

+   /* ALTER INDEX <name> ALTER COLUMN <colname> SET STATISTICS */
+   else if (HeadMatches("ALTER", "INDEX") && TailMatches("SET", "STATISTICS")){
+       /* We don't complete after "SET STATISTICS" */
+   }


Thanks,
Tatsuro Yamada

On Wed, Dec 19, 2018 at 04:26:27PM +0900, Tatsuro Yamada wrote:
> Yep, I already did that for ALTER INDEX in tab_completion_alter_index_set_statistics.patch like this:
>
> +   /* ALTER INDEX <name> ALTER COLUMN <colname> SET STATISTICS */
> +   else if (HeadMatches("ALTER", "INDEX") && TailMatches("SET", "STATISTICS")){
> +       /* We don't complete after "SET STATISTICS" */
> +   }

[Wake up, Neo]

Okay, then I propose to first extract a patch which does the following
things as a first step to simplify the follow-up work:
- No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of
schemas.
- Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS (now this
prints parameters, which is annoying).

Then let's figure out the details for the rest.
--
Michael

On 2018/12/19 18:22, Michael Paquier wrote:
> On Wed, Dec 19, 2018 at 04:26:27PM +0900, Tatsuro Yamada wrote:
>> Yep, I already did that for ALTER INDEX in tab_completion_alter_index_set_statistics.patch like this:
>>
>> +   /* ALTER INDEX <name> ALTER COLUMN <colname> SET STATISTICS */
>> +   else if (HeadMatches("ALTER", "INDEX") && TailMatches("SET", "STATISTICS")){
>> +       /* We don't complete after "SET STATISTICS" */
>> +   }
> 
> [Wake up, Neo]

Welcome to the real world. :)


> Okay, then I propose to first extract a patch which does the following
> things as a first step to simplify the follow-up work:
> - No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of
> schemas.
> - Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS (now this
> prints parameters, which is annoying).
> 
> Then let's figure out the details for the rest.

Alright, I'll create new patches including these:

   - No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of schema names
   - Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS by using *column_numbers*

After that,
I'll tackle to fix the documents for consistency, if possible.
   ====
   https://www.postgresql.org/docs/current/sql-altertable.html

     ALTER [ COLUMN ] column_name SET STATISTICS integer

   https://www.postgresql.org/docs/current/sql-alterindex.html

     ALTER [ COLUMN ] column_number SET STATISTICS integer
   ====


Thanks,
Tatsuro Yamada

On Thu, Dec 20, 2018 at 10:05:30AM +0900, Tatsuro Yamada wrote:
> Alright, I'll create new patches including these:
>
>   - No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of schema names
>   - Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS by
>   using *column_numbers*

Thanks for considering it!
--
Michael

On 2018/12/20 10:38, Michael Paquier wrote:
> On Thu, Dec 20, 2018 at 10:05:30AM +0900, Tatsuro Yamada wrote:
>> Alright, I'll create new patches including these:
>>
>>    - No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of schema names
>>    - Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS by
>>    using *column_numbers*
> 
> Thanks for considering it!

My pleasure, Neo. :)
Please wait for new WIP patches.

Thanks,
Tatsuro Yamada

On 2018/12/20 10:47, Tatsuro Yamada wrote:
> On 2018/12/20 10:38, Michael Paquier wrote:
>> On Thu, Dec 20, 2018 at 10:05:30AM +0900, Tatsuro Yamada wrote:
>>> Alright, I'll create new patches including these:
>>>
>>>    - No completion after "ALTER TABLE/INDEX SET STATISTICS" instead of schema names
>>>    - Complete "ALTER INDEX foo ALTER COLUMN SET" with STATISTICS by
>>>    using *column_numbers*
>>
>> Thanks for considering it!
> 
> My pleasure, Neo. :)
> Please wait for new WIP patches.

Attached file is a WIP patch.

*Example of after patching
========================================================
create table hoge (a integer, b integer, c integer);
create index ind_hoge on hoge(a, b, c, (c*1), (c*2), (c*3), (c*4), (c*5), (c*6), (c*7), (c*8), (c*9));

# \d+ ind_hoge
                     Index "public.ind_hoge"
  Column |  Type   | Key? | Definition | Storage | Stats target
--------+---------+------+------------+---------+--------------
  a      | integer | yes  | a          | plain   |
  b      | integer | yes  | b          | plain   |
  c      | integer | yes  | c          | plain   |
  expr   | integer | yes  | (c * 1)    | plain   |
  expr1  | integer | yes  | (c * 2)    | plain   |
  expr2  | integer | yes  | (c * 3)    | plain   |
  expr3  | integer | yes  | (c * 4)    | plain   |
  expr4  | integer | yes  | (c * 5)    | plain   |
  expr5  | integer | yes  | (c * 6)    | plain   |
  expr6  | integer | yes  | (c * 7)    | plain   |
  expr7  | integer | yes  | (c * 8)    | plain   |
  expr8  | integer | yes  | (c * 9)    | plain   |
btree, for table "public.hoge"

# alter index ind_hoge alter column <tab!>
1   10  11  12  2   3   4   5   6   7   8   9

# alter index ind_hoge alter column 1 <tab!>
1   10  11  12

# alter index ind_hoge alter column 10 SET STATISTICS <tab!>
<no completion!>

# alter index ind_hoge alter column 10 SET STATISTICS 100;
ALTER INDEX

# \d+ ind_hoge
                     Index "public.ind_hoge"
  Column |  Type   | Key? | Definition | Storage | Stats target
--------+---------+------+------------+---------+--------------
  a      | integer | yes  | a          | plain   |
  b      | integer | yes  | b          | plain   |
  c      | integer | yes  | c          | plain   |
  expr   | integer | yes  | (c * 1)    | plain   |
  expr1  | integer | yes  | (c * 2)    | plain   |
  expr2  | integer | yes  | (c * 3)    | plain   |
  expr3  | integer | yes  | (c * 4)    | plain   |
  expr4  | integer | yes  | (c * 5)    | plain   |
  expr5  | integer | yes  | (c * 6)    | plain   |
  expr6  | integer | yes  | (c * 7)    | plain   | 100
  expr7  | integer | yes  | (c * 8)    | plain   |
  expr8  | integer | yes  | (c * 9)    | plain   |
btree, for table "public.hoge"
========================================================

As you know above completed 1, 2 and 3 are not expression columns,
so it might better to remove these from the completion.
However, I didn't do that because a query for getting more suitable
attnum of index are became complicated.

Then, the patch includes new query to get attribute_numbers like this:

========================================================
+#define Query_for_list_of_attribute_numbers \
+"SELECT attnum "\
+"  FROM pg_catalog.pg_attribute a, "\
+"       pg_catalog.pg_class c "\
+" WHERE c.oid = a.attrelid "\
+"   AND a.attnum > 0 "\
+"   AND NOT a.attisdropped "\
+"   /* %d %s */" \
+"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') "\
+"   AND pg_catalog.pg_table_is_visible(c.oid) "\
+"order by a.attnum asc "
========================================================

I have a question.
I read following comment of _complete_from_query(), however I'm not sure whether "%d" is
needed or not in above query. Any advices welcome!

========================================================
  * 1. A simple query which must contain a %d and a %s, which will be replaced
  * by the string length of the text and the text itself. The query may also
  * have up to four more %s in it; the first two such will be replaced by the
  * value of completion_info_charp, the next two by the value of
  * completion_info_charp2.
========================================================

Thanks,
Tatsuro Yamada

On Fri, Dec 21, 2018 at 02:51:51PM +0900, Tatsuro Yamada wrote:
> Attached file is a WIP patch.

Before sorting out the exotic part of the feature, why not first fixing
the simple cases where we know that tab completion is broken and can be
improved?  This is what I meant in this email:
https://www.postgresql.org/message-id/20181219092255.GC680@paquier.xyz

The attached patch implements the idea.  What do you think?
--
Michael

On 2018/12/21 16:13, Michael Paquier wrote:
> On Fri, Dec 21, 2018 at 02:51:51PM +0900, Tatsuro Yamada wrote:
>> Attached file is a WIP patch.
> 
> Before sorting out the exotic part of the feature, why not first fixing
> the simple cases where we know that tab completion is broken and can be
> improved?  This is what I meant in this email:
> https://www.postgresql.org/message-id/20181219092255.GC680@paquier.xyz
> 
> The attached patch implements the idea.  What do you think?

Hmm... Okey, I agree.
Why I implemented the exotic part of the feature is that it is for user-friendly.
However, I suppose that user know the syntax because the syntax is used by an expert user.

Then, I got following messages when I patched your file on b981df4cc09aca978c5ce55e437a74913d09cccc,
so I rebased it. Please find attached file. :)
=======
$ patch -p1 < psql-tab-alter-column-stats-1.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/psql/tab-complete.c
Hunk #1 succeeded at 1601 (offset 35 lines).
Hunk #2 succeeded at 1920 (offset 35 lines).
=======

Thanks,
Tatsuro Yamada

On Tue, Dec 25, 2018 at 10:56:04AM +0900, Tatsuro Yamada wrote:
> Hmm... Okey, I agree.  Why I implemented the exotic part of the
> feature is that it is for user-friendly.  However, I suppose that
> user know the syntax because the syntax is used by an expert user.

Thanks, I have committed this one after making the logic to ignore
column numbers a bit smarter, one problem being that "ALTER INDEX foo
ALTER COLUMN" would try to suggest SET STATISTICS directly, which is
incorrect.  Instead I have tweaked the completion so as COLUMN is
added after "ALTER INDEX foo ALTER".  This could be completed later
with the column numbers, in a way similar to what ALTER TABLE does.
--
Michael

On Tue, Dec 25, 2018 at 02:27:03PM +0900, Michael Paquier wrote:
> Thanks, I have committed this one after making the logic to ignore
> column numbers a bit smarter, one problem being that "ALTER INDEX foo
> ALTER COLUMN" would try to suggest SET STATISTICS directly, which is
> incorrect.  Instead I have tweaked the completion so as COLUMN is
> added after "ALTER INDEX foo ALTER".  This could be completed later
> with the column numbers, in a way similar to what ALTER TABLE does.

Could you rebase the latest patch please?  The latest version sent
does not apply anymore:
[1]: https://www.postgresql.org/message-id/bcecaf0e-ab92-8271-6887-da213aea9dac@lab.ntt.co.jp
--
Michael

On 2018/12/25 14:27, Michael Paquier wrote:
> On Tue, Dec 25, 2018 at 10:56:04AM +0900, Tatsuro Yamada wrote:
>> Hmm... Okey, I agree.  Why I implemented the exotic part of the
>> feature is that it is for user-friendly.  However, I suppose that
>> user know the syntax because the syntax is used by an expert user.
> 
> Thanks, I have committed this one after making the logic to ignore
> column numbers a bit smarter, one problem being that "ALTER INDEX foo
> ALTER COLUMN" would try to suggest SET STATISTICS directly, which is
> incorrect.  Instead I have tweaked the completion so as COLUMN is
> added after "ALTER INDEX foo ALTER".  This could be completed later
> with the column numbers, in a way similar to what ALTER TABLE does.
> --
> Michael

Thanks for taking your time! :)

Cheers!
Tatsuro Yamada

On 2018/12/26 13:50, Michael Paquier wrote:
> On Tue, Dec 25, 2018 at 02:27:03PM +0900, Michael Paquier wrote:
>> Thanks, I have committed this one after making the logic to ignore
>> column numbers a bit smarter, one problem being that "ALTER INDEX foo
>> ALTER COLUMN" would try to suggest SET STATISTICS directly, which is
>> incorrect.  Instead I have tweaked the completion so as COLUMN is
>> added after "ALTER INDEX foo ALTER".  This could be completed later
>> with the column numbers, in a way similar to what ALTER TABLE does.
> 
> Could you rebase the latest patch please?  The latest version sent
> does not apply anymore:
> [1]: https://www.postgresql.org/message-id/bcecaf0e-ab92-8271-6887-da213aea9dac@lab.ntt.co.jp
> --
> Michael

Do you mean my "fix_manual_of_alter_index_v2.patch"?
It is able to patch on f89ae34ab8b4d9e9ce8af6bd889238b0ccff17cb like this:

=====
$ patch -p1 < fix_manual_of_alter_index_v2.patch
patching file doc/src/sgml/ref/alter_index.sgml
=====

Thanks,
Tatsuro Yamada

On Wed, Dec 26, 2018 at 02:05:26PM +0900, Tatsuro Yamada wrote:
> Do you mean my "fix_manual_of_alter_index_v2.patch"?

Nope.  This patch is only a proposal for the documentation.  The main
patch to extend psql completion so as column numbers are suggested
fails to apply.
--
Michael

On 2018/12/26 14:15, Michael Paquier wrote:
> On Wed, Dec 26, 2018 at 02:05:26PM +0900, Tatsuro Yamada wrote:
>> Do you mean my "fix_manual_of_alter_index_v2.patch"?
> 
> Nope.  This patch is only a proposal for the documentation.  The main
> patch to extend psql completion so as column numbers are suggested
> fails to apply.

I rebased the WIP patch. :)

* Following query is added to get attribute numbers of index,
   however its result contains not only expression columns but also other columns.

* I'm not sure what should I use "%d" and first "%s" in the query, so I commented out: /* %d %s */.
   I know this is ugly.. Do you know how to use?

+#define Query_for_list_of_attribute_numbers \
+"SELECT attnum "\
+"  FROM pg_catalog.pg_attribute a, "\
+"       pg_catalog.pg_class c "\
+" WHERE c.oid = a.attrelid "\
+"   AND a.attnum > 0 "\
+"   AND NOT a.attisdropped "\
+"   /* %d %s */" \
+"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') "\
+"   AND pg_catalog.pg_table_is_visible(c.oid) "\
+"order by a.attnum asc "

Thanks,
Tatsuro Yamada

On 2018/12/26 14:50, Tatsuro Yamada wrote:
> On 2018/12/26 14:15, Michael Paquier wrote:
>> On Wed, Dec 26, 2018 at 02:05:26PM +0900, Tatsuro Yamada wrote:
>>> Do you mean my "fix_manual_of_alter_index_v2.patch"?
>>
>> Nope.  This patch is only a proposal for the documentation.  The main
>> patch to extend psql completion so as column numbers are suggested
>> fails to apply.
> 
> I rebased the WIP patch. :)
> 
> * Following query is added to get attribute numbers of index,
>    however its result contains not only expression columns but also other columns.
> 
> * I'm not sure what should I use "%d" and first "%s" in the query, so I commented out: /* %d %s */.
>    I know this is ugly.. Do you know how to use?
> 
> +#define Query_for_list_of_attribute_numbers \
> +"SELECT attnum "\
> +"  FROM pg_catalog.pg_attribute a, "\
> +"       pg_catalog.pg_class c "\
> +" WHERE c.oid = a.attrelid "\
> +"   AND a.attnum > 0 "\
> +"   AND NOT a.attisdropped "\
> +"   /* %d %s */" \
> +"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') "\
> +"   AND pg_catalog.pg_table_is_visible(c.oid) "\
> +"order by a.attnum asc "

I modified the patch to remove unusable condition.

========
# create table hoge (a integer, b integer, c integer);
# create index ind_hoge on hoge(a, b, c, (c*1), (c*2), (c*3), (c*4), (c*5), (c*6), (c*7), (c*8), (c*9));

# \d ind_hoge
        Index "public.ind_hoge"
  Column |  Type   | Key? | Definition
--------+---------+------+------------
  a      | integer | yes  | a
  b      | integer | yes  | b
  c      | integer | yes  | c
  expr   | integer | yes  | (c * 1)
  expr1  | integer | yes  | (c * 2)
  expr2  | integer | yes  | (c * 3)
  expr3  | integer | yes  | (c * 4)
  expr4  | integer | yes  | (c * 5)
  expr5  | integer | yes  | (c * 6)
  expr6  | integer | yes  | (c * 7)
  expr7  | integer | yes  | (c * 8)
  expr8  | integer | yes  | (c * 9)
btree, for table "public.hoge"

# alter index ind_hoge alter column <tab!>
1   10  11  12  2   3   4   5   6   7   8   9

# alter index ind_hoge alter column 1 <tab!>
1   10  11  12

# alter index ind_hoge alter column 10 <tab!>
alter index ind_hoge alter COLUMN 10 SET STATISTICS
========

Thanks,
Tatsuro Yamada

On 26/12/2018 07:07, Tatsuro Yamada wrote:
> +#define Query_for_list_of_attribute_numbers \
> +"SELECT attnum "\
> +"  FROM pg_catalog.pg_attribute a, "\
> +"       pg_catalog.pg_class c "\
> +" WHERE c.oid = a.attrelid "\
> +"   AND a.attnum > 0 "\
> +"   AND NOT a.attisdropped "\
> +"   /* %d %s */" \
> +"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') "\
> +"   AND pg_catalog.pg_table_is_visible(c.oid) "\
> +"order by a.attnum asc "

This needs a bit of refinement.  You need to handle quoted index names
(see nearby Query_for_list_of_attributes), and you should also complete
partial numbers (e.g., if I type 1 then complete 10, 11, ... if
appropriate).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Peter,

On 2019/01/25 20:09, Peter Eisentraut wrote:
> On 26/12/2018 07:07, Tatsuro Yamada wrote:
>> +#define Query_for_list_of_attribute_numbers \
>> +"SELECT attnum "\
>> +"  FROM pg_catalog.pg_attribute a, "\
>> +"       pg_catalog.pg_class c "\
>> +" WHERE c.oid = a.attrelid "\
>> +"   AND a.attnum > 0 "\
>> +"   AND NOT a.attisdropped "\
>> +"   /* %d %s */" \
>> +"   AND a.attrelid = (select oid from pg_catalog.pg_class where relname = '%s') "\
>> +"   AND pg_catalog.pg_table_is_visible(c.oid) "\
>> +"order by a.attnum asc "
> 
> This needs a bit of refinement.  You need to handle quoted index names
> (see nearby Query_for_list_of_attributes), and you should also complete
> partial numbers (e.g., if I type 1 then complete 10, 11, ... if
> appropriate).


Thanks for the comments.
I modified the patch to handle the both:
   - quoted index names
   - complete partial numbers

e.g.
-----
# create table hoge (a integer, b integer, c integer);
# create index ind_hoge on hoge(a, b, c, (c*1), (c*2), (c*3), (c*4), (c*5), (c*6), (c*7), (c*8), (c*9));
# create index "ind hoge2" on hoge(c, b, a, (c*1), (c*2), (c*3), (c*4), (c*5), (c*6), (c*7), (c*8), (c*9));

# alter index "ind hoge2" alter column
1   10  11  12  2   3   4   5   6   7   8   9
# alter index "ind hoge2" alter column 1
1   10  11  12

# alter index ind_hoge alter column
1   10  11  12  2   3   4   5   6   7   8   9
# alter index ind_hoge alter column 1
1   10  11  12
-----

Please find attached file. :)

Regards,
Tatsuro Yamada

On Mon, Jan 28, 2019 at 02:18:25PM +0900, Tatsuro Yamada wrote:
> I modified the patch to handle the both:
>   - quoted index names
>   - complete partial numbers

Committed, which should close the loop for this thread.  If you have
suggestions for the documentation, maybe it would be better to start
another thread.  I am not sure if that's worth worrying about, but
others may have input to offer on the matter.
--
Michael

Hi Michael,

On 2019/01/28 15:31, Michael Paquier wrote:
> On Mon, Jan 28, 2019 at 02:18:25PM +0900, Tatsuro Yamada wrote:
>> I modified the patch to handle the both:
>>    - quoted index names
>>    - complete partial numbers
> 
> Committed, which should close the loop for this thread.  If you have
> suggestions for the documentation, maybe it would be better to start
> another thread.  I am not sure if that's worth worrying about, but
> others may have input to offer on the matter.

Thanks!

I'll send a documentation patch on another thread to hear any opinions.

Regards,
Tatsuro Yamada