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

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");
+   /*
+    * Complete DECLARE <name> ... CURSOR with one of WITH HOLD, WITHOUT HOLD,
+    * and FOR.
+    */
    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/