Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
To: Masahiko Sawada <sawada.mshk@gmail.com>, Shinya11.Kato@nttdata.com
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <b0e4c5c53ef84c5395524f5056fc71f0@MP-MSGSS-MBX001.msg.nttdata.co.jp>
 <CAD21AoBRw9FY6oNj8ubY7XEwJ9Bprfehu88_ePbCfH93JLxWsw@mail.gmail.com>
 <7559e8ee8a0644c8aad79837092143c7@MP-MSGSS-MBX001.msg.nttdata.co.jp>
 <0eab839e-5827-e517-4abf-00b3e581cd45@oss.nttdata.com>
 <CAD21AoBiGkvFpizGAWt-8mQEja3_Afu0pjn09vR1Gk7d+zj_Cw@mail.gmail.com>
 <697532b0-0a83-7046-1a58-b1ee0f66ac9a@oss.nttdata.com>
 <7bf2e1700a7345f79c04fd513675467a@MP-MSGSS-MBX001.msg.nttdata.co.jp>
 <CAD21AoDR5BiPjTpWsc-EmG0+_Gt43Poc0G3wjBpw1kgHhiSvWA@mail.gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <a562a2cb-e2dd-9c02-d849-a52db9cf6907@oss.nttdata.com>
Date: Thu, 7 Jan 2021 10:59:28 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: 
 <CAD21AoDR5BiPjTpWsc-EmG0+_Gt43Poc0G3wjBpw1kgHhiSvWA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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