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

On Wed, Dec 9, 2020 at 12:59 PM <Shinya11.Kato@nttdata.com> 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/

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

On 2021/01/05 15:02, Shinya11.Kato@nttdata.com 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

On Tue, Jan 5, 2021 at 3:03 PM <Shinya11.Kato@nttdata.com> 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/

On Tue, Jan 5, 2021 at 6:08 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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/

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

On Tue, Jan 5, 2021 at 6:08 PM Fujii Masao <masao.fujii@oss.nttdata.com> 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

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

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");
+   /*
+    * 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/

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

On Wed, Jan 6, 2021 at 3:37 PM <Shinya11.Kato@nttdata.com> wrote:

+#define Query_for_list_of_cursors \
+" SELECT name FROM pg_cursors"\

This query should be the following?

" SELECT pg_catalog.quote_ident(name) "\
" FROM pg_catalog.pg_cursors "\
" WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"

+/* CLOSE */
+      else if (Matches("CLOSE"))
+              COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+                                                      " UNION ALL SELECT 'ALL'");

"UNION ALL" should be "UNION"?

Thank you for your advice, and I corrected them.

+               COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+                                                       " UNION SELECT 'ABSOLUTE'"
+                                                       " UNION SELECT 'BACKWARD'"
+                                                       " UNION SELECT 'FORWARD'"
+                                                       " UNION SELECT 'RELATIVE'"
+                                                       " UNION SELECT 'ALL'"
+                                                       " UNION SELECT 'NEXT'"
+                                                       " UNION SELECT 'PRIOR'"
+                                                       " UNION SELECT 'FIRST'"
+                                                       " UNION SELECT 'LAST'"
+                                                       " UNION SELECT 'FROM'"
+                                                       " UNION SELECT 'IN'");

This change makes psql unexpectedly output "FROM" and "IN" just after "FETCH".

I think "FROM" and "IN" can be placed just after "FETCH". According to
the documentation, the direction can be empty. For instance, we can do
like:

Thank you!

I've attached the patch improving the tab completion for DECLARE as an
example. What do you think?

BTW according to the documentation, the options of DECLARE statement
(BINARY, INSENSITIVE, SCROLL, and NO SCROLL) are order-sensitive.

DECLARE name [ BINARY ] [ INSENSITIVE ] [ [ NO ] SCROLL ]
CURSOR [ { WITH | WITHOUT } HOLD ] FOR query

But I realized that these options are actually order-insensitive. For
instance, we can declare a cursor like:

=# declare abc scroll binary cursor for select * from pg_class;
DECLARE CURSOR

The both parser code and documentation has been unchanged from 2003.
Is it a documentation bug?

Thank you for your patch, and it is good.
I cannot find the description "(BINARY, INSENSITIVE, SCROLL, and NO SCROLL) are order-sensitive."
I saw "The key words BINARY, INSENSITIVE, and SCROLL can appear in any order." , according to the documentation.

Thanks, you're right. I missed that sentence. I still don't think the
syntax of DECLARE statement in the documentation doesn't match its
implementation but I agree that it's order-insensitive.

I made a new patch, but the amount of codes was large due to order-insensitive.

Thank you for updating the patch!

Yeah, I'm also afraid a bit that conditions will exponentially
increase when a new option is added to DECLARE statement in the
future. Looking at the parser code for DECLARE statement, we can put
the same options multiple times (that's also why I don't think it
matches). For instance,

postgres(1:44758)=# begin;
BEGIN
postgres(1:44758)=# declare test binary binary binary cursor for
select * from pg_class;
DECLARE CURSOR

So how about simplify the above code as follows?

@@ -3005,8 +3014,23 @@ psql_completion(const char *text, int start, int end)
else if (Matches("DECLARE", MatchAny))
COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
"CURSOR");
+   /*
+    * Complete DECLARE <name> with one of BINARY, INSENSITIVE, SCROLL,
+    * NO SCROLL, and CURSOR. The tail doesn't match any keywords for
+    * DECLARE, assume we want options.
+    */
+   else if (HeadMatches("DECLARE", MatchAny, "*") &&
+            TailMatches(MatchAnyExcept("CURSOR|WITH|WITHOUT|HOLD|FOR")))
+       COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
+                     "CURSOR");

This change seems to cause "DECLARE ... CURSOR FOR SELECT <tab>" to
unexpectedly output BINARY, INSENSITIVE, etc.

Regards,

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

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?

@@ -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/

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

On Thu, Jan 7, 2021 at 1:30 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

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.

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/

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

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

On Thu, Jan 7, 2021 at 9:32 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

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?

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/

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.

On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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/

On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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

On Tue, Jan 12, 2021 at 11:09 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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

On Wed, Jan 13, 2021 at 1:55 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 12, 2021 at 11:09 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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.

Agreed.

I updated the patch that way. Could you review this version?

Thank you for updating the patch. Looks good to me.

Regards,

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

On 2021/01/14 14:38, Masahiko Sawada wrote:

On Wed, Jan 13, 2021 at 1:55 PM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 12, 2021 at 11:09 AM Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Jan 12, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jan 11, 2021 at 11:00 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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.

Agreed.

I updated the patch that way. Could you review this version?

Thank you for updating the patch. Looks good to me.

Pushed. Thanks!

Regards,

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