On Thu, Jan 7, 2021 at 1:30 PM Fujii Masao <masao.fu...@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.fu...@oss.nttdata.com>
> > wrote:
> >>
> >>
> >>
> >> On 2021/01/07 10:01, Masahiko Sawada wrote:
> >>> On Wed, Jan 6, 2021 at 3:37 PM <shinya11.k...@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/
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 9dcab0d2fa..5838319676 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,37 @@ 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 with more options */
+ else if (HeadMatches("DECLARE", MatchAny, "BINARY|INSENSITIVE|SCROLL|NO") &&
+ TailMatches("BINARY|INSENSITIVE|SCROLL|NO"))
+ {
+ /* Complete DECLARE <name> [options] NO with SCROLL */
+ if (TailMatches("NO"))
+ COMPLETE_WITH("SCROLL");
+ /*
+ * 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 (TailMatches("BINARY|INSENSITIVE|SCROLL"))
+ COMPLETE_WITH("BINARY", "INSENSITIVE", "SCROLL", "NO SCROLL", "CURSOR");
+ }
+ /* 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 +3202,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 +3236,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 */
