[PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Shinya11.Kato

Hi!

 

I created a patch for improving CLOSE, FETCH, MOVE tab completion.

Specifically, I add CLOSE, FETCH, MOVE tab completion for completing a predefined cursors.

 

Regards,

Shinya Kato


fix_tab_complete_close_fetch_move.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Wed, Dec 9, 2020 at 12:59 PM <[hidden email]> wrote:
>
> Hi!
>
>
>
> I created a patch for improving CLOSE, FETCH, MOVE tab completion.
>
> Specifically, I add CLOSE, FETCH, MOVE tab completion for completing a predefined cursors.
>

Thank you for the patch!

When I applied the patch, I got the following whitespace warnings:

$ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
indent with spaces.
        COMPLETE_WITH_QUERY(Query_for_list_of_cursors
/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
indent with spaces.
                            " UNION SELECT 'ABSOLUTE'"
/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
indent with spaces.
                            " UNION SELECT 'BACKWARD'"
/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
indent with spaces.
                            " UNION SELECT 'FORWARD'"
/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
indent with spaces.
                            " UNION SELECT 'RELATIVE'"
warning: squelched 19 whitespace errors
warning: 24 lines add whitespace errors.

I recommend you checking whitespaces or running pgindent.

Here are some comments:

    /*
-    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD, RELATIVE, ALL,
-    * NEXT, PRIOR, FIRST, LAST
+    * Complete FETCH with a list of cursors and one of ABSOLUTE,
BACKWARD, FORWARD, RELATIVE, ALL,
+    * NEXT, PRIOR, FIRST, LAST, FROM, IN
     */

Maybe I think the commend should say:

+    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD, RELATIVE, ALL,
+    * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors

Other updates of the comment seem to have the same issue.

Or I think we can omit the details from the comment since it seems
redundant information. We can read it from the arguments of the
following COMPLETE_WITH_QUERY().

---
-   /*
-    * Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
-    * but we may as well tab-complete both: perhaps some users prefer one
-    * variant or the other.
-    */
+   /* Complete FETCH <direction> with a list of cursors and "FROM" or "IN" */

Why did you remove the second sentence in the above comment?

---
The patch improves tab completion for CLOSE, FETCH, and MOVE but is
there any reason why you didn't do that for DECLARE? I think DECLARE
also can be improved and it's a good timing for that.

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/


Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Shinya11.Kato
Thank you for your review!
I fixed some codes and attach a new patch.

>When I applied the patch, I got the following whitespace warnings:
>
>$ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
>/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
>indent with spaces.
>        COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
>indent with spaces.
>                            " UNION SELECT 'ABSOLUTE'"
>/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
>indent with spaces.
>                            " UNION SELECT 'BACKWARD'"
>/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
>indent with spaces.
>                            " UNION SELECT 'FORWARD'"
>/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
>indent with spaces.
>                            " UNION SELECT 'RELATIVE'"
>warning: squelched 19 whitespace errors
>warning: 24 lines add whitespace errors.
>
>I recommend you checking whitespaces or running pgindent.
Thank you for your advice and I have corrected it.

>Here are some comments:
>
>    /*
>-    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>RELATIVE, ALL,
>-    * NEXT, PRIOR, FIRST, LAST
>+    * Complete FETCH with a list of cursors and one of ABSOLUTE,
>BACKWARD, FORWARD, RELATIVE, ALL,
>+    * NEXT, PRIOR, FIRST, LAST, FROM, IN
>     */
>
>Maybe I think the commend should say:
>
>+    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>RELATIVE, ALL,
>+    * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
>
>Other updates of the comment seem to have the same issue.
>
>Or I think we can omit the details from the comment since it seems redundant
>information. We can read it from the arguments of the following
>COMPLETE_WITH_QUERY().
It certainly seems redundant, so I deleted them.

>---
>-   /*
>-    * Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
>-    * but we may as well tab-complete both: perhaps some users prefer one
>-    * variant or the other.
>-    */
>+   /* Complete FETCH <direction> with a list of cursors and "FROM" or
>+ "IN" */
>
>Why did you remove the second sentence in the above comment?
I had misunderstood the meaning and deleted it.
I deleted it as well as above, but would you prefer it to be there?

>---
>The patch improves tab completion for CLOSE, FETCH, and MOVE but is there
>any reason why you didn't do that for DECLARE? I think DECLARE also can be
>improved and it's a good timing for that.

I wanted to improve tab completion for DECLARE, but I couldn't find anything to improve.
Please let me know if there are any codes that can be improved.

Regards,
Shinya Kato

fix_tab_complete_close_fetch_move_v2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-4


On 2021/01/05 15:02, [hidden email] wrote:
> Thank you for your review!
> I fixed some codes and attach a new patch.

Thanks for updating the patch!

+#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"?

+ 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".

>
>> When I applied the patch, I got the following whitespace warnings:
>>
>> $ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
>> indent with spaces.
>>         COMPLETE_WITH_QUERY(Query_for_list_of_cursors
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
>> indent with spaces.
>>                             " UNION SELECT 'ABSOLUTE'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
>> indent with spaces.
>>                             " UNION SELECT 'BACKWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
>> indent with spaces.
>>                             " UNION SELECT 'FORWARD'"
>> /home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
>> indent with spaces.
>>                             " UNION SELECT 'RELATIVE'"
>> warning: squelched 19 whitespace errors
>> warning: 24 lines add whitespace errors.
>>
>> I recommend you checking whitespaces or running pgindent.
>
> Thank you for your advice and I have corrected it.
>
>> Here are some comments:
>>
>>     /*
>> -    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> -    * NEXT, PRIOR, FIRST, LAST
>> +    * Complete FETCH with a list of cursors and one of ABSOLUTE,
>> BACKWARD, FORWARD, RELATIVE, ALL,
>> +    * NEXT, PRIOR, FIRST, LAST, FROM, IN
>>      */
>>
>> Maybe I think the commend should say:
>>
>> +    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
>> RELATIVE, ALL,
>> +    * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
>>
>> Other updates of the comment seem to have the same issue.
>>
>> Or I think we can omit the details from the comment since it seems redundant
>> information. We can read it from the arguments of the following
>> COMPLETE_WITH_QUERY().
>
> It certainly seems redundant, so I deleted them.

I think that it's better to update and keep those comments rather than removing them.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
In reply to this post by Shinya11.Kato
On Tue, Jan 5, 2021 at 3:03 PM <[hidden email]> wrote:
>
> Thank you for your review!
> I fixed some codes and attach a new patch.

Thank you for updating the patch!

>
> >When I applied the patch, I got the following whitespace warnings:
> >
> >$ git apply ~/patches/fix_tab_complete_close_fetch_move.patch
> >/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:40:
> >indent with spaces.
> >        COMPLETE_WITH_QUERY(Query_for_list_of_cursors
> >/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:41:
> >indent with spaces.
> >                            " UNION SELECT 'ABSOLUTE'"
> >/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:42:
> >indent with spaces.
> >                            " UNION SELECT 'BACKWARD'"
> >/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:43:
> >indent with spaces.
> >                            " UNION SELECT 'FORWARD'"
> >/home/masahiko/patches/fix_tab_complete_close_fetch_move.patch:44:
> >indent with spaces.
> >                            " UNION SELECT 'RELATIVE'"
> >warning: squelched 19 whitespace errors
> >warning: 24 lines add whitespace errors.
> >
> >I recommend you checking whitespaces or running pgindent.
>
> Thank you for your advice and I have corrected it.
>
> >Here are some comments:
> >
> >    /*
> >-    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
> >RELATIVE, ALL,
> >-    * NEXT, PRIOR, FIRST, LAST
> >+    * Complete FETCH with a list of cursors and one of ABSOLUTE,
> >BACKWARD, FORWARD, RELATIVE, ALL,
> >+    * NEXT, PRIOR, FIRST, LAST, FROM, IN
> >     */
> >
> >Maybe I think the commend should say:
> >
> >+    * Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD,
> >RELATIVE, ALL,
> >+    * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
> >
> >Other updates of the comment seem to have the same issue.
> >
> >Or I think we can omit the details from the comment since it seems redundant
> >information. We can read it from the arguments of the following
> >COMPLETE_WITH_QUERY().
>
> It certainly seems redundant, so I deleted them.
>
> >---
> >-   /*
> >-    * Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
> >-    * but we may as well tab-complete both: perhaps some users prefer one
> >-    * variant or the other.
> >-    */
> >+   /* Complete FETCH <direction> with a list of cursors and "FROM" or
> >+ "IN" */
> >
> >Why did you remove the second sentence in the above comment?
>
> I had misunderstood the meaning and deleted it.
> I deleted it as well as above, but would you prefer it to be there?
I would leave it. I realized this area is recently updated by commit
8176afd8b7. In that change, the comments were updated rather than
removed. So it might be better to leave them. Sorry for confusing you.

>
> >---
> >The patch improves tab completion for CLOSE, FETCH, and MOVE but is there
> >any reason why you didn't do that for DECLARE? I think DECLARE also can be
> >improved and it's a good timing for that.
>
> I wanted to improve tab completion for DECLARE, but I couldn't find anything to improve.
> Please let me know if there are any codes that can be improved.

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?

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

tab_completion_declare_stmt.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
In reply to this post by Fujii Masao-4
On Tue, Jan 5, 2021 at 6:08 PM Fujii Masao <[hidden email]> wrote:

>
>
> +               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:

postgres(1:7668)=# begin;
BEGIN

postgres(1:7668)=# declare test cursor for select relname from pg_class;
DECLARE CURSOR

postgres(1:7668)=# fetch from test;
   relname
--------------
 pg_statistic
(1 row)

postgres(1:7668)=# fetch in test;
 relname
---------
 pg_type
(1 row)

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-4


On 2021/01/06 11:13, Masahiko Sawada wrote:

> On Tue, Jan 5, 2021 at 6:08 PM Fujii Masao <[hidden email]> wrote:
>>
>>
>> +               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.

You're right. Thanks for correcting me!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Shinya11.Kato
>+#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.

I made a new patch, but the amount of codes was large due to order-insensitive.
If you know of a better way, please let me know.

Regards,
Shinya Kato

fix_tab_complete_close_fetch_move_v3.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Wed, Jan 6, 2021 at 3:37 PM <[hidden email]> 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");
+   /*
+    * Complete DECLARE <name> ... CURSOR with one of WITH HOLD, WITHOUT HOLD,
+    * and FOR.
+    */
    else if (HeadMatches("DECLARE") && TailMatches("CURSOR"))
        COMPLETE_WITH("WITH HOLD", "WITHOUT HOLD", "FOR");
+   else if (HeadMatches("DECLARE") && TailMatches("HOLD"))
+       COMPLETE_WITH("FOR");

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-4


On 2021/01/07 10:01, Masahiko Sawada wrote:

> On Wed, Jan 6, 2021 at 3:37 PM <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Thu, Jan 7, 2021 at 10:59 AM Fujii Masao <[hidden email]> wrote:

>
>
>
> On 2021/01/07 10:01, Masahiko Sawada wrote:
> > On Wed, Jan 6, 2021 at 3:37 PM <[hidden email]> 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?

@@ -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");
    else if (HeadMatches("DECLARE") && TailMatches("CURSOR"))
        COMPLETE_WITH("WITH HOLD", "WITHOUT HOLD", "FOR");
+   else if (HeadMatches("DECLARE") && TailMatches("HOLD"))
+       COMPLETE_WITH("FOR");


Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-4


On 2021/01/07 12:42, Masahiko Sawada wrote:

> On Thu, Jan 7, 2021 at 10:59 AM Fujii Masao <[hidden email]> wrote:
>>
>>
>>
>> On 2021/01/07 10:01, Masahiko Sawada wrote:
>>> On Wed, Jan 6, 2021 at 3:37 PM <[hidden email]> 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 | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Thu, Jan 7, 2021 at 1:30 PM Fujii Masao <[hidden email]> wrote:

>
>
>
> On 2021/01/07 12:42, Masahiko Sawada wrote:
> > On Thu, Jan 7, 2021 at 10:59 AM Fujii Masao <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 2021/01/07 10:01, Masahiko Sawada wrote:
> >>> On Wed, Jan 6, 2021 at 3:37 PM <[hidden email]> 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.


Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

fix_tab_complete_close_fetch_move_v4.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Shinya11.Kato
>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.

Thank you very much for the new patch!
I checked the operation and I think it is good.

Regards,
Shinya Kato
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-4


On 2021/01/07 17:28, [hidden email] 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

fix_tab_complete_close_fetch_move_v5.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Thu, Jan 7, 2021 at 9:32 PM Fujii Masao <[hidden email]> wrote:

>
>
>
> On 2021/01/07 17:28, [hidden email] 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/


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Peter Eisentraut-7
In reply to this post by Masahiko Sawada
On 2021-01-05 10:56, Masahiko Sawada wrote:

> 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?

According to the SQL standard, the ordering of the cursor properties is
fixed.  Even if the PostgreSQL parser offers more flexibility, I think
we should continue to encourage writing the clauses in the standard order.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Masahiko Sawada
On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<[hidden email]> wrote:

>
> On 2021-01-05 10:56, Masahiko Sawada wrote:
> > 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?
>
> According to the SQL standard, the ordering of the cursor properties is
> fixed.  Even if the PostgreSQL parser offers more flexibility, I think
> we should continue to encourage writing the clauses in the standard order.

Thanks for your comment. Agreed.

So regarding the tab completion for DECLARE statement, perhaps it
would be better to follow the documentation? In the current proposed
patch, we complete it with the options in any order.

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-2
On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <[hidden email]> wrote:

>
> On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
> <[hidden email]> wrote:
> >
> > On 2021-01-05 10:56, Masahiko Sawada wrote:
> > > 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?
> >
> > According to the SQL standard, the ordering of the cursor properties is
> > fixed.  Even if the PostgreSQL parser offers more flexibility, I think
> > we should continue to encourage writing the clauses in the standard order.
>
> Thanks for your comment. Agreed.
>
> So regarding the tab completion for DECLARE statement, perhaps it
> would be better to follow the documentation?

IMO yes because it's less confusing to make the document and
tab-completion consistent.

Regards,

--
Fujii Masao


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Feature improvement for CLOSE, FETCH, MOVE tab completion

Fujii Masao-2
On Tue, Jan 12, 2021 at 11:09 AM Fujii Masao <[hidden email]> wrote:

>
> On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <[hidden email]> wrote:
> >
> > On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
> > <[hidden email]> wrote:
> > >
> > > On 2021-01-05 10:56, Masahiko Sawada wrote:
> > > > 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?
> > >
> > > According to the SQL standard, the ordering of the cursor properties is
> > > fixed.  Even if the PostgreSQL parser offers more flexibility, I think
> > > we should continue to encourage writing the clauses in the standard order.
> >
> > Thanks for your comment. Agreed.
> >
> > So regarding the tab completion for DECLARE statement, perhaps it
> > would be better to follow the documentation?
>
> IMO yes because it's less confusing to make the document and
> tab-completion consistent.
I updated the patch that way. Could you review this version?

Regards,

--
Fujii Masao

fix_tab_complete_close_fetch_move_v6.patch (6K) Download Attachment
12