Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion |
Date | |
Msg-id | a562a2cb-e2dd-9c02-d849-a52db9cf6907@oss.nttdata.com Whole thread Raw |
In response to | Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion (Masahiko Sawada <sawada.mshk@gmail.com>) |
Responses |
Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
(Masahiko Sawada <sawada.mshk@gmail.com>)
|
List | pgsql-hackers |
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. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
pgsql-hackers by date: