Re: Tab completion for ALTER INDEX|TABLE ALTER COLUMN SET STATISTICS

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