Обсуждение: Re: psql UPDATE field [tab] expands to DEFAULT?

[ moving thread to -hackers ]

So I propose the attached patch for fixing the clear bugs that have
emerged in this discussion: don't confuse UPDATE ... SET ... with
GUC-setting commands, and don't offer just DEFAULT in contexts where
that's unlikely to be the only valid completion.

Nosing around in tab-complete.c, I notice a fair number of other
places where we're doing COMPLETE_WITH() with just a single possible
completion.  Knowing what we know now, in each one of those places
libreadline will suppose that that completion is the only syntactically
legal continuation, and throw away anything else the user might've typed.
We should probably inspect each of those places to see if that's really
desirable behavior ... but I didn't muster the energy to do that this
morning.

            regards, tom lane

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5e38f46..3530570 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -3361,8 +3361,13 @@ psql_completion(const char *text, int start, int end)
     else if (HeadMatches("ALTER", "DATABASE|FUNCTION|PROCEDURE|ROLE|ROUTINE|USER") &&
              TailMatches("SET", MatchAny))
         COMPLETE_WITH("FROM CURRENT", "TO");
-    /* Suggest possible variable values */
-    else if (TailMatches("SET", MatchAny, "TO|="))
+
+    /*
+     * Suggest possible variable values in SET variable TO|=, along with the
+     * preceding ALTER syntaxes.
+     */
+    else if (TailMatches("SET", MatchAny, "TO|=") &&
+             !TailMatches("UPDATE", MatchAny, "SET", MatchAny, "TO|="))
     {
         /* special cased code for individual GUCs */
         if (TailMatches("DateStyle", "TO|="))
@@ -3390,8 +3395,18 @@ psql_completion(const char *text, int start, int end)
             else if (guctype && strcmp(guctype, "bool") == 0)
                 COMPLETE_WITH("on", "off", "true", "false", "yes", "no",
                               "1", "0", "DEFAULT");
-            else
-                COMPLETE_WITH("DEFAULT");
+
+            /*
+             * Note: if we didn't recognize the GUC name, it's important to
+             * not offer any completions, as most likely we've misinterpreted
+             * the context and this isn't a GUC-setting command at all.  If we
+             * did recognize the GUC but it's not enum or bool type, it still
+             * seems better to do nothing.  We used to offer (only) "DEFAULT"
+             * as a possible completion, but that turns out to be a bad idea,
+             * because with a single completion libreadline will decide that
+             * that's the only legal text and throw away whatever the user
+             * might've typed already.
+             */

             if (guctype)
                 free(guctype);

I wrote:
> Nosing around in tab-complete.c, I notice a fair number of other
> places where we're doing COMPLETE_WITH() with just a single possible
> completion.  Knowing what we know now, in each one of those places
> libreadline will suppose that that completion is the only syntactically
> legal continuation, and throw away anything else the user might've typed.
> We should probably inspect each of those places to see if that's really
> desirable behavior ... but I didn't muster the energy to do that this
> morning.

I took a closer look and realized that this isn't some magic behavior of
arcane parts of libreadline; it's more like self-inflicted damage.  It
happens because tab-complete.c's complete_from_const() is doing exactly
what its comment says it does:

/*
 * This function returns one fixed string the first time even if it doesn't
 * match what's there, and nothing the second time. This should be used if
 * there is only one possibility that can appear at a certain spot, so
 * misspellings will be overwritten.  The string to be passed must be in
 * completion_charp.
 */

This is unlike complete_from_list(), which will only return completions
that match the text-string-so-far.

I have to wonder whether complete_from_const()'s behavior is really
a good idea; I think there might be an argument for getting rid of it
and using complete_from_list() even for one-element lists.

We certainly didn't do anybody any favors in the refactoring we did in
4f3b38fe2, which removed the source-code difference between calling
complete_from_const() and calling complete_from_list() with just one list
item.  But even before that, I really doubt that many people hacking on
tab-complete.c had internalized the idea that COMPLETE_WITH_CONST()
implied a higher degree of certainty than COMPLETE_WITH_LIST() with one
list item.  I'm pretty sure I'd never understood that.

Both of those functions go back to the beginnings of tab-complete.c,
so there's not much available in the history to explain the difference
in behavior (and the discussion of the original patch, if any, is lost
to the mists of time --- our archives for pgsql-patches only go back to
2000).  But my own feeling about this is that there's no situation in
which I'd expect tab completion to wipe out text I'd typed.

Thoughts?

            regards, tom lane

I wrote:
> I took a closer look and realized that this isn't some magic behavior of
> arcane parts of libreadline; it's more like self-inflicted damage.  It
> happens because tab-complete.c's complete_from_const() is doing exactly
> what its comment says it does:

> /*
>  * This function returns one fixed string the first time even if it doesn't
>  * match what's there, and nothing the second time. This should be used if
>  * there is only one possibility that can appear at a certain spot, so
>  * misspellings will be overwritten.  The string to be passed must be in
>  * completion_charp.
>  */

> This is unlike complete_from_list(), which will only return completions
> that match the text-string-so-far.

> I have to wonder whether complete_from_const()'s behavior is really
> a good idea; I think there might be an argument for getting rid of it
> and using complete_from_list() even for one-element lists.

I experimented with ripping out complete_from_const() altogether, and
soon found that there's still one place where we need it: down at the
end of psql_completion, where we've failed to find any useful completion.
If that instance of COMPLETE_WITH("") is implemented by complete_from_list
then readline will happily try to do filename completion :-(.
(I don't quite understand why we don't get the wiping-out behavior there;
maybe an empty-string result is treated differently from
not-empty-string?)

So I propose the attached instead, which doesn't get rid of
complete_from_const but ensures that it's only used in that one place.

This is independent of the other patch shown upthread.  I'm proposing
this one for HEAD only but would back-patch the other.

            regards, tom lane

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7dcf342..d41f923 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -205,19 +205,22 @@ do { \
     matches = completion_matches(text, complete_from_versioned_schema_query); \
 } while (0)
 
+/*
+ * Caution: COMPLETE_WITH_CONST is not for general-purpose use; you probably
+ * want COMPLETE_WITH() with one element, instead.
+ */
+#define COMPLETE_WITH_CONST(cs, con) \
+do { \
+    completion_case_sensitive = (cs); \
+    completion_charp = (con); \
+    matches = completion_matches(text, complete_from_const); \
+} while (0)
+
 #define COMPLETE_WITH_LIST_INT(cs, list) \
 do { \
     completion_case_sensitive = (cs); \
-    if (!(list)[1]) \
-    { \
-        completion_charp = (list)[0]; \
-        matches = completion_matches(text, complete_from_const); \
-    } \
-    else \
-    { \
-        completion_charpp = (list); \
-        matches = completion_matches(text, complete_from_list); \
-    } \
+    completion_charpp = (list); \
+    matches = completion_matches(text, complete_from_list); \
 } while (0)
 
 #define COMPLETE_WITH_LIST(list) COMPLETE_WITH_LIST_INT(false, list)
@@ -3745,7 +3748,7 @@ psql_completion(const char *text, int start, int end)
      */
     if (matches == NULL)
     {
-        COMPLETE_WITH("");
+        COMPLETE_WITH_CONST(true, "");
 #ifdef HAVE_RL_COMPLETION_APPEND_CHARACTER
         rl_completion_append_character = '\0';
 #endif
@@ -4177,8 +4180,10 @@ complete_from_list(const char *text, int state)
  * This function returns one fixed string the first time even if it doesn't
  * match what's there, and nothing the second time. This should be used if
  * there is only one possibility that can appear at a certain spot, so
- * misspellings will be overwritten.  The string to be passed must be in
- * completion_charp.
+ * misspellings will be overwritten.  (Caution: in practice that's almost
+ * never a good idea.  We currently use this function only for forcing
+ * readline to not attempt filename completion by default.)  The string
+ * to be passed must be in completion_charp.
  */
 static char *
 complete_from_const(const char *text, int state)