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

On Thu, Jan 7, 2021 at 10:59 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2021/01/07 10:01, Masahiko Sawada wrote:
> > On Wed, Jan 6, 2021 at 3:37 PM <Shinya11.Kato@nttdata.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?

@@ -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");
    else if (HeadMatches("DECLARE") && TailMatches("CURSOR"))
        COMPLETE_WITH("WITH HOLD", "WITHOUT HOLD", "FOR");
+   else if (HeadMatches("DECLARE") && TailMatches("HOLD"))
+       COMPLETE_WITH("FOR");


Regards,

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