From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Shinya11.Kato@nttdata.com
To: sawada.mshk@gmail.com
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
Date: Thu, 7 Jan 2021 21:32:05 +0900
Message-ID: <a24d9125-cec9-885d-cf9b-3e71bd62e15e@oss.nttdata.com> (raw)
In-Reply-To: <0b48b5dab6a8431aaed016931666cbc1@MP-MSGSS-MBX001.msg.nttdata.co.jp>
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>
<80b6a07a-9249-bf08-6add-bdda1ccb0659@oss.nttdata.com>
<CAD21AoCJ4+f6KMT4Srw+oK8NV6TCAOQBAkZ93vs+C-8Hq5cbDQ@mail.gmail.com>
<0b48b5dab6a8431aaed016931666cbc1@MP-MSGSS-MBX001.msg.nttdata.co.jp>
On 2021/01/07 17:28, Shinya11.Kato@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?
If this is true, I'd like to refactor the code a bit.
What about the attached patch?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachments:fix_tab_complete_close_fetch_move_v5.patch (text/plain, 4.5K) downloaddiff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.cindex 9dcab0d2fa..aa994a3af0 100644--- a/src/bin/psql/tab-complete.c+++ b/src/bin/psql/tab-complete.c@@ -976,6 +976,11 @@ static const SchemaQuery Query_for_list_of_statistics = {
" and pg_catalog.pg_table_is_visible(c2.oid)"\
" and c2.relispartition = 'true'"
+#define Query_for_list_of_cursors \+" SELECT pg_catalog.quote_ident(name) "\+" FROM pg_catalog.pg_cursors "\+" WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"+
/*
* These object types were introduced later than our support cutoff of
* server version 7.4. We use the VersionedQuery infrastructure so that
@@ -2284,6 +2289,10 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_VERSIONED_SCHEMA_QUERY(Query_for_list_of_procedures, NULL);
else if (Matches("CALL", MatchAny))
COMPLETE_WITH("(");
+/* CLOSE */+ else if (Matches("CLOSE"))+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors+ " UNION SELECT 'ALL'");
/* CLUSTER */
else if (Matches("CLUSTER"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables, "UNION SELECT 'VERBOSE'");
@@ -3002,11 +3011,39 @@ psql_completion(const char *text, int start, int end)
" UNION SELECT 'ALL'");
/* DECLARE */
++ /*+ * Complete DECLARE <name> with one of BINARY, INSENSITIVE, SCROLL, NO+ * SCROLL, and CURSOR.+ */
else if (Matches("DECLARE", MatchAny))
COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
"CURSOR");
++ /*+ * Complete DECLARE <name> [options] 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 (HeadMatches("DECLARE") && TailMatches("BINARY|INSENSITIVE|SCROLL"))+ COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",+ "CURSOR");+ /* Complete DECLARE <name> [options] NO with SCROLL */+ else if (HeadMatches("DECLARE") && TailMatches("NO"))+ COMPLETE_WITH("SCROLL");++ /*+ * Complete DECLARE ... CURSOR with one of WITH HOLD, WITHOUT HOLD, and+ * FOR+ */
else if (HeadMatches("DECLARE") && TailMatches("CURSOR"))
COMPLETE_WITH("WITH HOLD", "WITHOUT HOLD", "FOR");
+ /* Complete DECLARE ... CURSOR WITH|WITHOUT with HOLD */+ else if (HeadMatches("DECLARE") && TailMatches("CURSOR", "WITH|WITHOUT"))+ COMPLETE_WITH("HOLD");+ /* Complete DECLARE ... CURSOR WITH|WITHOUT HOLD with FOR */+ else if (HeadMatches("DECLARE") && TailMatches("CURSOR", "WITH|WITHOUT", "HOLD"))+ COMPLETE_WITH("FOR");
/* DELETE --- can be inside EXPLAIN, RULE, etc */
/* ... despite which, only complete DELETE with FROM at start of line */
@@ -3167,15 +3204,31 @@ psql_completion(const char *text, int start, int end)
/*
* Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD, RELATIVE, ALL,
- * NEXT, PRIOR, FIRST, LAST+ * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
*/
else if (Matches("FETCH|MOVE"))
- COMPLETE_WITH("ABSOLUTE", "BACKWARD", "FORWARD", "RELATIVE",- "ALL", "NEXT", "PRIOR", "FIRST", "LAST");+ 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'");- /* Complete FETCH BACKWARD or FORWARD with one of ALL, FROM, IN */+ /*+ * Complete FETCH BACKWARD or FORWARD with one of ALL, FROM, IN, and a+ * list of cursors+ */
else if (Matches("FETCH|MOVE", "BACKWARD|FORWARD"))
- COMPLETE_WITH("ALL", "FROM", "IN");+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors+ " UNION SELECT 'ALL'"+ " UNION SELECT 'FROM'"+ " UNION SELECT 'IN'");
/*
* Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
@@ -3185,7 +3238,13 @@ psql_completion(const char *text, int start, int end)
else if (Matches("FETCH|MOVE", "ABSOLUTE|BACKWARD|FORWARD|RELATIVE",
MatchAnyExcept("FROM|IN")) ||
Matches("FETCH|MOVE", "ALL|NEXT|PRIOR|FIRST|LAST"))
- COMPLETE_WITH("FROM", "IN");+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors+ " UNION SELECT 'FROM'"+ " UNION SELECT 'IN'");+ /* Complete FETCH <direction> "FROM" or "IN" with a list of cursors */+ else if (HeadMatches("FETCH|MOVE") &&+ TailMatches("FROM|IN"))+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors);
/* FOREIGN DATA WRAPPER */
/* applies in ALTER/DROP FDW and in CREATE SERVER */
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, Shinya11.Kato@nttdata.com, sawada.mshk@gmail.com
Subject: Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion
In-Reply-To: <a24d9125-cec9-885d-cf9b-3e71bd62e15e@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