RE: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion - Mailing list pgsql-hackers

>On Thu, Jan 7, 2021 at 1:30 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>
>> On 2021/01/07 12:42, Masahiko Sawada wrote:
>> > On Thu, Jan 7, 2021 at 10:59 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> >>
>> >> On 2021/01/07 10:01, Masahiko Sawada wrote:
>> >>> On Wed, Jan 6, 2021 at 3:37 PM <Shinya11(dot)Kato(at)nttdata(dot)com> wrote:
>> >>>>
>> >>>>> +#define Query_for_list_of_cursors \
>> >>>>> +" SELECT name FROM pg_cursors"\
>> >>>>>
>> >>>>> This query should be the following?
>> >>>>>
>> >>>>> " SELECT pg_catalog.quote_ident(name) "\
>> >>>>> " FROM pg_catalog.pg_cursors "\
>> >>>>> " WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"
>> >>>>>
>> >>>>> +/* CLOSE */
>> >>>>> + else if (Matches("CLOSE"))
>> >>>>> + COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>> >>>>> + " UNION ALL SELECT 'ALL'");
>> >>>>>
>> >>>>> "UNION ALL" should be "UNION"?
>> >>>>
>> >>>> Thank you for your advice, and I corrected them.
>> >>>>
>> >>>>>> + COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>> >>>>>> + " UNION SELECT 'ABSOLUTE'"
>> >>>>>> + " UNION SELECT 'BACKWARD'"
>> >>>>>> + " UNION SELECT 'FORWARD'"
>> >>>>>> + " UNION SELECT 'RELATIVE'"
>> >>>>>> + " UNION SELECT 'ALL'"
>> >>>>>> + " UNION SELECT 'NEXT'"
>> >>>>>> + " UNION SELECT 'PRIOR'"
>> >>>>>> + " UNION SELECT 'FIRST'"
>> >>>>>> + " UNION SELECT 'LAST'"
>> >>>>>> + " UNION SELECT 'FROM'"
>> >>>>>> + " UNION SELECT 'IN'");
>> >>>>>>
>> >>>>>> This change makes psql unexpectedly output "FROM" and "IN" just after "FETCH".
>> >>>>>
>> >>>>> I think "FROM" and "IN" can be placed just after "FETCH". According to
>> >>>>> the documentation, the direction can be empty. For instance, we can do
>> >>>>> like:
>> >>>>
>> >>>> Thank you!
>> >>>>
>> >>>>> I've attached the patch improving the tab completion for DECLARE as an
>> >>>>> example. What do you think?
>> >>>>>
>> >>>>> BTW according to the documentation, the options of DECLARE statement
>> >>>>> (BINARY, INSENSITIVE, SCROLL, and NO SCROLL) are order-sensitive.
>> >>>>>
>> >>>>> DECLARE name [ BINARY ] [ INSENSITIVE ] [ [ NO ] SCROLL ]
>> >>>>> CURSOR [ { WITH | WITHOUT } HOLD ] FOR query
>> >>>>>
>> >>>>> But I realized that these options are actually order-insensitive. For
>> >>>>> instance, we can declare a cursor like:
>> >>>>>
>> >>>>> =# declare abc scroll binary cursor for select * from pg_class;
>> >>>>> DECLARE CURSOR
>> >>>>>
>> >>>>> The both parser code and documentation has been unchanged from 2003.
>> >>>>> Is it a documentation bug?
>> >>>>
>> >>>> Thank you for your patch, and it is good.
>> >>>> I cannot find the description "(BINARY, INSENSITIVE, SCROLL, and NO SCROLL) are order-sensitive."
>> >>>> I saw "The key words BINARY, INSENSITIVE, and SCROLL can appear in any order." , according to the
documentation.
>> >>>
>> >>> Thanks, you're right. I missed that sentence. I still don't think the
>> >>> syntax of DECLARE statement in the documentation doesn't match its
>> >>> implementation but I agree that it's order-insensitive.
>> >>>
>> >>>> I made a new patch, but the amount of codes was large due to order-insensitive.
>> >>>
>> >>> Thank you for updating the patch!
>> >>>
>> >>> Yeah, I'm also afraid a bit that conditions will exponentially
>> >>> increase when a new option is added to DECLARE statement in the
>> >>> future. Looking at the parser code for DECLARE statement, we can put
>> >>> the same options multiple times (that's also why I don't think it
>> >>> matches). For instance,
>> >>>
>> >>> postgres(1:44758)=# begin;
>> >>> BEGIN
>> >>> postgres(1:44758)=# declare test binary binary binary cursor for
>> >>> select * from pg_class;
>> >>> DECLARE CURSOR
>> >>>
>> >>> So how about simplify the above code as follows?
>> >>>
>> >>> @@ -3005,8 +3014,23 @@ psql_completion(const char *text, int start, int end)
>> >>> else if (Matches("DECLARE", MatchAny))
>> >>> COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
>> >>> "CURSOR");
>> >>> + /*
>> >>> + * Complete DECLARE <name> with one of BINARY, INSENSITIVE, SCROLL,
>> >>> + * NO SCROLL, and CURSOR. The tail doesn't match any keywords for
>> >>> + * DECLARE, assume we want options.
>> >>> + */
>> >>> + else if (HeadMatches("DECLARE", MatchAny, "*") &&
>> >>> + TailMatches(MatchAnyExcept("CURSOR|WITH|WITHOUT|HOLD|FOR")))
>> >>> + COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
>> >>> + "CURSOR");
>> >>
>> >> This change seems to cause "DECLARE ... CURSOR FOR SELECT <tab>" to
>> >> unexpectedly output BINARY, INSENSITIVE, etc.
>> >
>> > Indeed. Is the following not complete but much better?
>>
>> Yes, I think that's better.
>>
>> >
>> > @@ -3002,11 +3011,18 @@ psql_completion(const char *text, int start, int end)
>> > " UNION SELECT 'ALL'");
>> >
>> > /* DECLARE */
>> > - else if (Matches("DECLARE", MatchAny))
>> > - COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
>> > - "CURSOR");
>> > + /*
>> > + * Complete DECLARE <name> with one of BINARY, INSENSITIVE, SCROLL,
>> > + * NO SCROLL, and CURSOR. If the tail is any one of options, assume we
>> > + * still want options.
>> > + */
>> > + else if (Matches("DECLARE", MatchAny) ||
>> > + TailMatches("BINARY|INSENSITIVE|SCROLL|NO"))
>> > + COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL", "CURSOR");
>>
>> This change seems to cause "DECLARE ... NO <tab>" to unexpectedly output
>> "NO SCROLL". Also this change seems to cause "COPY ... (FORMAT BINARY <tab>"
>> to unexpectedly output BINARY, CURSOR, etc.
>
>Oops, I missed the HeadMatches(). Thank you for pointing this out.
>
>I've attached the updated patch including Kato-san's changes. I
>think the tab completion support for DECLARE added by this patch
>works better.

Thank you very much for the new patch!
I checked the operation and I think it is good.

Regards,
Shinya Kato