Обсуждение: tab completion of enum values is broken

This doesn't work anymore:

create type e2 as enum ('foo', 'bar');
alter type e2 rename value 'b<TAB>

This now results in

alter type e2 rename value 'b'

Bisecting blames

commit cd69ec66c88633c09bc9a984a7f0930e09c7c96e
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Thu Jan 23 11:07:12 2020 -0500

     Improve psql's tab completion for filenames.

which did deal with quoting of things to be completed, so it seems very 
plausible to be some collateral damage.

The queries issued by the completion engine are

bad

LOG:  statement: SELECT pg_catalog.quote_literal(enumlabel)   FROM 
pg_catalog.pg_enum e, pg_catalog.pg_type t  WHERE t.oid = e.enumtypid 
AND substring(pg_catalog.quote_literal(enumlabel),1,1)='b'    AND 
(pg_catalog.quote_ident(typname)='e2'         OR '"' || typname || 
'"'='e2')    AND pg_catalog.pg_type_is_visible(t.oid)    LIMIT 1000

good

LOG:  statement: SELECT pg_catalog.quote_literal(enumlabel)   FROM 
pg_catalog.pg_enum e, pg_catalog.pg_type t  WHERE t.oid = e.enumtypid 
AND substring(pg_catalog.quote_literal(enumlabel),1,2)='''b'    AND 
(pg_catalog.quote_ident(typname)='e2'         OR '"' || typname || 
'"'='e2')    AND pg_catalog.pg_type_is_visible(t.oid)    LIMIT 1000

I tried quickly fiddling with the substring() call to correct for the 
changed offset somehow, but didn't succeed.  Needs more analysis.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> This doesn't work anymore:
> create type e2 as enum ('foo', 'bar');
> alter type e2 rename value 'b<TAB>
> This now results in
> alter type e2 rename value 'b'

Ugh.  I'll take a look.

            regards, tom lane

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> This doesn't work anymore:
> create type e2 as enum ('foo', 'bar');
> alter type e2 rename value 'b<TAB>
> This now results in
> alter type e2 rename value 'b'

The main issue here is that Query_for_list_of_enum_values[_with_schema]
is designed to match against a pre-quoted list of enum values,
which was appropriate when it was written because we hadn't configured
Readline to do anything special with quotes.  Now that we have, the
string that is being supplied to match against lacks the leading quote,
so that we need to remove the quote_literal() calls from those queries.

A secondary problem is that if we fail to identify any match, Readline
nonetheless decides to append a trailing quote.  That's why you get the
added quote above, and it still happens with the query fix.  I'm not
sure why Readline thinks it should do that.  I worked around it in the
attached draft patch by setting rl_completion_suppress_quote = 1 in our
failure-to-match case, but that feels like using a big hammer rather
than a proper solution.

I'm not totally satisfied with this patch for a couple of reasons:

1. It'll allow the user to enter a non-quoted enum value,
    for example alter type e2 rename value b<TAB>
produces
    alter type e2 rename value bar 
It's not clear to me that there's any way around that, though.
I tried returning pre-quoted values as we did before (ie,
changing only the WHERE clauses in the queries) but then
Readline fails to match anything.  We do have code to force
quoting of actual filenames, but I think that's dependent on
going through rl_filename_completion_function(), which of course
we can't do here.

2. It doesn't seem like there's any nice way to deal with enum
values that contain single quotes (which need to be doubled).
Admittedly the use-case for that is probably epsilon, but
it annoys me that it doesn't work.

In the end, it seems like the value of this specific completion
rule is not large enough to justify doing a ton of work to
eliminate #1 or #2.  So I propose doing the attached and calling
it good.  Maybe we could add a test case.

Oh ... experimenting on macOS (with the system-provided libedit)
shows no bug here.  So I guess we'll need to make this conditional
somehow, perhaps on USE_FILENAME_QUOTING_FUNCTIONS.  That's another
reason for not going overboard.

            regards, tom lane

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 39be6f556a..e856d918ae 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -679,20 +679,20 @@ static const SchemaQuery Query_for_list_of_collations = {
 "        OR '\"' || nspname || '\"' ='%s') "

 #define Query_for_list_of_enum_values \
-"SELECT pg_catalog.quote_literal(enumlabel) "\
+"SELECT enumlabel "\
 "  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
 " WHERE t.oid = e.enumtypid "\
-"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND substring(enumlabel,1,%d)='%s' "\
 "   AND (pg_catalog.quote_ident(typname)='%s' "\
 "        OR '\"' || typname || '\"'='%s') "\
 "   AND pg_catalog.pg_type_is_visible(t.oid)"

 #define Query_for_list_of_enum_values_with_schema \
-"SELECT pg_catalog.quote_literal(enumlabel) "\
+"SELECT enumlabel "\
 "  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
 " WHERE t.oid = e.enumtypid "\
 "   AND n.oid = t.typnamespace "\
-"   AND substring(pg_catalog.quote_literal(enumlabel),1,%d)='%s' "\
+"   AND substring(enumlabel,1,%d)='%s' "\
 "   AND (pg_catalog.quote_ident(typname)='%s' "\
 "        OR '\"' || typname || '\"'='%s') "\
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
@@ -4413,8 +4413,12 @@ psql_completion(const char *text, int start, int end)
     if (matches == NULL)
     {
         COMPLETE_WITH_CONST(true, "");
+        /* Also, prevent Readline from appending stuff to the non-match */
 #ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
         rl_completion_append_character = '\0';
+#endif
+#ifdef HAVE_RL_COMPLETION_SUPPRESS_QUOTE
+        rl_completion_suppress_quote = 1;
 #endif
     }

I wrote:
> Oh ... experimenting on macOS (with the system-provided libedit)
> shows no bug here.  So I guess we'll need to make this conditional
> somehow, perhaps on USE_FILENAME_QUOTING_FUNCTIONS.  That's another
> reason for not going overboard.

After further fooling with that, I concluded that the only workable
solution is a run-time check for whether the readline library included
the leading quote in what it hands us.  A big advantage of doing it this
way is that it mostly fixes my complaint #1: by crafting the check
properly, we will include quotes if the user hits TAB without having typed
anything, and we won't complete an incorrectly non-quoted identifier.
There's still nothing to be done about single quotes inside an enum
label, but I'm okay with blowing that case off.

So I think the attached is committable.  I've tried it on readline
7.0 (RHEL8) as well as whatever libedit Apple is currently shipping.

            regards, tom lane

diff --git a/src/bin/psql/t/010_tab_completion.pl b/src/bin/psql/t/010_tab_completion.pl
index ba98b8190b..d3d1bd650e 100644
--- a/src/bin/psql/t/010_tab_completion.pl
+++ b/src/bin/psql/t/010_tab_completion.pl
@@ -42,7 +42,8 @@ $node->start;
 $node->safe_psql('postgres',
         "CREATE TABLE tab1 (f1 int, f2 text);\n"
       . "CREATE TABLE mytab123 (f1 int, f2 text);\n"
-      . "CREATE TABLE mytab246 (f1 int, f2 text);\n");
+      . "CREATE TABLE mytab246 (f1 int, f2 text);\n"
+      . "CREATE TYPE enum1 AS ENUM ('foo', 'bar', 'baz');\n");

 # Developers would not appreciate this test adding a bunch of junk to
 # their ~/.psql_history, so be sure to redirect history into a temp file.
@@ -223,6 +224,16 @@ check_completion(

 clear_line();

+# check enum label completion
+# some versions of readline/libedit require two tabs here, some only need one
+# also, some versions will offer quotes, some will not
+check_completion(
+    "ALTER TYPE enum1 RENAME VALUE 'ba\t\t",
+    qr|'?bar'? +'?baz'?|,
+    "offer multiple enum choices");
+
+clear_line();
+
 # send psql an explicit \q to shut it down, else pty won't close properly
 $timer->start(5);
 $in .= "\\q\n";
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 39be6f556a..f7ce90da6e 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -278,25 +278,40 @@ do { \
     matches = rl_completion_matches(text, complete_from_query); \
 } while (0)

+/*
+ * libedit will typically include the literal's leading single quote in
+ * "text", while readline will not.  Adapt our offered strings to fit.
+ * If we don't yet have text, include quotes in what's offered, to get the
+ * user off to the right start.
+ */
 #define COMPLETE_WITH_ENUM_VALUE(type) \
 do { \
     char   *_completion_schema; \
     char   *_completion_type; \
+    bool    use_quotes; \
 \
     _completion_schema = strtokx(type, " \t\n\r", ".", "\"", 0, \
                                  false, false, pset.encoding); \
     (void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
                    false, false, pset.encoding); \
     _completion_type = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
-                               false, false, pset.encoding);  \
-    if (_completion_type == NULL)\
+                               false, false, pset.encoding); \
+    use_quotes = (text[0] == '\'' || \
+                  start == 0 || rl_line_buffer[start - 1] != '\''); \
+    if (_completion_type == NULL) \
     { \
-        completion_charp = Query_for_list_of_enum_values; \
+        if (use_quotes) \
+            completion_charp = Query_for_list_of_enum_values_quoted; \
+        else \
+            completion_charp = Query_for_list_of_enum_values_unquoted; \
         completion_info_charp = type; \
     } \
     else \
     { \
-        completion_charp = Query_for_list_of_enum_values_with_schema; \
+        if (use_quotes) \
+            completion_charp = Query_for_list_of_enum_values_with_schema_quoted; \
+        else \
+            completion_charp = Query_for_list_of_enum_values_with_schema_unquoted; \
         completion_info_charp = _completion_type; \
         completion_info_charp2 = _completion_schema; \
     } \
@@ -678,7 +693,7 @@ static const SchemaQuery Query_for_list_of_collations = {
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
 "        OR '\"' || nspname || '\"' ='%s') "

-#define Query_for_list_of_enum_values \
+#define Query_for_list_of_enum_values_quoted \
 "SELECT pg_catalog.quote_literal(enumlabel) "\
 "  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
 " WHERE t.oid = e.enumtypid "\
@@ -687,7 +702,16 @@ static const SchemaQuery Query_for_list_of_collations = {
 "        OR '\"' || typname || '\"'='%s') "\
 "   AND pg_catalog.pg_type_is_visible(t.oid)"

-#define Query_for_list_of_enum_values_with_schema \
+#define Query_for_list_of_enum_values_unquoted \
+"SELECT enumlabel "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t "\
+" WHERE t.oid = e.enumtypid "\
+"   AND substring(enumlabel,1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"        OR '\"' || typname || '\"'='%s') "\
+"   AND pg_catalog.pg_type_is_visible(t.oid)"
+
+#define Query_for_list_of_enum_values_with_schema_quoted \
 "SELECT pg_catalog.quote_literal(enumlabel) "\
 "  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
 " WHERE t.oid = e.enumtypid "\
@@ -698,6 +722,17 @@ static const SchemaQuery Query_for_list_of_collations = {
 "   AND (pg_catalog.quote_ident(nspname)='%s' "\
 "        OR '\"' || nspname || '\"' ='%s') "

+#define Query_for_list_of_enum_values_with_schema_unquoted \
+"SELECT enumlabel "\
+"  FROM pg_catalog.pg_enum e, pg_catalog.pg_type t, pg_catalog.pg_namespace n "\
+" WHERE t.oid = e.enumtypid "\
+"   AND n.oid = t.typnamespace "\
+"   AND substring(enumlabel,1,%d)='%s' "\
+"   AND (pg_catalog.quote_ident(typname)='%s' "\
+"        OR '\"' || typname || '\"'='%s') "\
+"   AND (pg_catalog.quote_ident(nspname)='%s' "\
+"        OR '\"' || nspname || '\"' ='%s') "
+
 #define Query_for_list_of_template_databases \
 "SELECT pg_catalog.quote_ident(d.datname) "\
 "  FROM pg_catalog.pg_database d "\
@@ -4413,8 +4448,12 @@ psql_completion(const char *text, int start, int end)
     if (matches == NULL)
     {
         COMPLETE_WITH_CONST(true, "");
+        /* Also, prevent Readline from appending stuff to the non-match */
 #ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
         rl_completion_append_character = '\0';
+#endif
+#ifdef HAVE_RL_COMPLETION_SUPPRESS_QUOTE
+        rl_completion_suppress_quote = 1;
 #endif
     }