On Thu, Jan 7, 2021 at 9:32 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote: > > > > On 2021/01/07 17:28, shinya11.k...@nttdata.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/