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