Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

On Thu, Jan 7, 2021 at 9:32 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
>
> On 2021/01/07 17:28, Shinya11(dot)Kato(at)nttdata(dot)com wrote:
> >> 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.
>
> Thanks!
>
> + /* Complete with more options */
> + else if (HeadMatches("DECLARE", MatchAny, "BINARY|INSENSITIVE|SCROLL|NO") &&
> + TailMatches("BINARY|INSENSITIVE|SCROLL|NO"))
>
> Seems "MatchAny, "BINARY|INSENSITIVE|SCROLL|NO"" is not necessary. Right?
>

Right.

> If this is true, I'd like to refactor the code a bit.
> What about the attached patch?

Thank you for updating the patch! Looks good to me.

Regards,

--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/