From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Shinya11.Kato@nttdata.com, PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
Date: Thu, 7 Jan 2021 13:30:32 +0900
Message-ID: <80b6a07a-9249-bf08-6add-bdda1ccb0659@oss.nttdata.com> (raw)
In-Reply-To: <CAD21AoDOqcJM160sEuiUHrub4TxLQ8ZtUswauBVTyNYEqRsmdw@mail.gmail.com>
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>
<a562a2cb-e2dd-9c02-d849-a52db9cf6907@oss.nttdata.com>
<CAD21AoDOqcJM160sEuiUHrub4TxLQ8ZtUswauBVTyNYEqRsmdw@mail.gmail.com>
On 2021/01/07 12:42, Masahiko Sawada wrote:
> 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?
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.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, sawada.mshk@gmail.com
Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
In-Reply-To: <80b6a07a-9249-bf08-6add-bdda1ccb0659@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox