On 2021/01/07 17:28, shinya11.k...@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
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9dcab0d2fa..aa994a3af0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -976,6 +976,11 @@ static const SchemaQuery Query_for_list_of_statistics = {
" and pg_catalog.pg_table_is_visible(c2.oid)"\
" and c2.relispartition = 'true'"
+#define Query_for_list_of_cursors \
+" SELECT pg_catalog.quote_ident(name) "\
+" FROM pg_catalog.pg_cursors "\
+" WHERE substring(pg_catalog.quote_ident(name),1,%d)='%s'"
+
/*
* These object types were introduced later than our support cutoff of
* server version 7.4. We use the VersionedQuery infrastructure so that
@@ -2284,6 +2289,10 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_VERSIONED_SCHEMA_QUERY(Query_for_list_of_procedures, NULL);
else if (Matches("CALL", MatchAny))
COMPLETE_WITH("(");
+/* CLOSE */
+ else if (Matches("CLOSE"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION SELECT 'ALL'");
/* CLUSTER */
else if (Matches("CLUSTER"))
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_clusterables,
"UNION SELECT 'VERBOSE'");
@@ -3002,11 +3011,39 @@ psql_completion(const char *text, int start, int end)
" UNION SELECT 'ALL'");
/* DECLARE */
+
+ /*
+ * Complete DECLARE <name> with one of BINARY, INSENSITIVE, SCROLL, NO
+ * SCROLL, and CURSOR.
+ */
else if (Matches("DECLARE", MatchAny))
COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
"CURSOR");
+
+ /*
+ * Complete DECLARE <name> [options] with one of BINARY, INSENSITIVE,
+ * SCROLL, NO SCROLL, and CURSOR. If the tail is any one of options,
+ * assume we still want options.
+ */
+ else if (HeadMatches("DECLARE") &&
TailMatches("BINARY|INSENSITIVE|SCROLL"))
+ COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL",
+ "CURSOR");
+ /* Complete DECLARE <name> [options] NO with SCROLL */
+ else if (HeadMatches("DECLARE") && TailMatches("NO"))
+ COMPLETE_WITH("SCROLL");
+
+ /*
+ * Complete DECLARE ... CURSOR with one of WITH HOLD, WITHOUT HOLD, and
+ * FOR
+ */
else if (HeadMatches("DECLARE") && TailMatches("CURSOR"))
COMPLETE_WITH("WITH HOLD", "WITHOUT HOLD", "FOR");
+ /* Complete DECLARE ... CURSOR WITH|WITHOUT with HOLD */
+ else if (HeadMatches("DECLARE") && TailMatches("CURSOR",
"WITH|WITHOUT"))
+ COMPLETE_WITH("HOLD");
+ /* Complete DECLARE ... CURSOR WITH|WITHOUT HOLD with FOR */
+ else if (HeadMatches("DECLARE") && TailMatches("CURSOR",
"WITH|WITHOUT", "HOLD"))
+ COMPLETE_WITH("FOR");
/* DELETE --- can be inside EXPLAIN, RULE, etc */
/* ... despite which, only complete DELETE with FROM at start of line */
@@ -3167,15 +3204,31 @@ psql_completion(const char *text, int start, int end)
/*
* Complete FETCH with one of ABSOLUTE, BACKWARD, FORWARD, RELATIVE,
ALL,
- * NEXT, PRIOR, FIRST, LAST
+ * NEXT, PRIOR, FIRST, LAST, FROM, IN, and a list of cursors
*/
else if (Matches("FETCH|MOVE"))
- COMPLETE_WITH("ABSOLUTE", "BACKWARD", "FORWARD", "RELATIVE",
- "ALL", "NEXT", "PRIOR", "FIRST",
"LAST");
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION SELECT
'ABSOLUTE'"
+ " UNION SELECT
'BACKWARD'"
+ " UNION SELECT
'FORWARD'"
+ " UNION SELECT
'RELATIVE'"
+ " UNION SELECT 'ALL'"
+ " UNION SELECT 'NEXT'"
+ " UNION SELECT 'PRIOR'"
+ " UNION SELECT 'FIRST'"
+ " UNION SELECT 'LAST'"
+ " UNION SELECT 'FROM'"
+ " UNION SELECT 'IN'");
- /* Complete FETCH BACKWARD or FORWARD with one of ALL, FROM, IN */
+ /*
+ * Complete FETCH BACKWARD or FORWARD with one of ALL, FROM, IN, and a
+ * list of cursors
+ */
else if (Matches("FETCH|MOVE", "BACKWARD|FORWARD"))
- COMPLETE_WITH("ALL", "FROM", "IN");
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION SELECT 'ALL'"
+ " UNION SELECT 'FROM'"
+ " UNION SELECT 'IN'");
/*
* Complete FETCH <direction> with "FROM" or "IN". These are equivalent,
@@ -3185,7 +3238,13 @@ psql_completion(const char *text, int start, int end)
else if (Matches("FETCH|MOVE", "ABSOLUTE|BACKWARD|FORWARD|RELATIVE",
MatchAnyExcept("FROM|IN")) ||
Matches("FETCH|MOVE", "ALL|NEXT|PRIOR|FIRST|LAST"))
- COMPLETE_WITH("FROM", "IN");
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors
+ " UNION SELECT 'FROM'"
+ " UNION SELECT 'IN'");
+ /* Complete FETCH <direction> "FROM" or "IN" with a list of cursors */
+ else if (HeadMatches("FETCH|MOVE") &&
+ TailMatches("FROM|IN"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_cursors);
/* FOREIGN DATA WRAPPER */
/* applies in ALTER/DROP FDW and in CREATE SERVER */
