Обсуждение: 15beta1 tab completion of extension versions

Jeff Janes <jeff.janes@gmail.com> writes:
> Extension version strings need to be quoted.  Either double or single
> quotes will work.  In released psql clients, tab completion offers double
> quoted suggestions:
> But commit 02b8048ba5 broke that, it now offers unquoted version strings
> which if used as offered then lead to syntax errors.

Ooops.

> The code change seems to have been intentional, but I don't think the
> behavior change was intended.

Given the comments about it, I'm sure I tested the behavior somewhere
along the line --- but I must not have done so with the final logic
of _complete_from_query.

> Putting pg_catalog.quote_ident back
> into Query_for_list_of_available_extension_versions* fixes it, but might
> not be the best way to fix it.

Yeah, that seems like the appropriate fix.  Done, thanks for the report!

            regards, tom lane

I wrote:
> Jeff Janes <jeff.janes@gmail.com> writes:
>> Putting pg_catalog.quote_ident back
>> into Query_for_list_of_available_extension_versions* fixes it, but might
>> not be the best way to fix it.

> Yeah, that seems like the appropriate fix.  Done, thanks for the report!

Actually ... after further thought it seems like maybe we should
make this more like other cases rather than less so.  ISTM that much
of the issue here is somebody's decision that "TO version" should be
offered as a completion of "UPDATE", which is unlike the way we do this
anywhere else --- the usual thing is to offer "UPDATE TO" as a single
completion.  So I'm thinking about the attached.

This behaves a little differently from the old code.  In v14,
    alter extension pg_trgm upd<TAB>
gives you
    alter extension pg_trgm update<space>
and another <TAB> produces
    alter extension pg_trgm update TO "1.

With this,
    alter extension pg_trgm upd<TAB>
gives you
    alter extension pg_trgm update to<space>
and another <TAB> produces
    alter extension pg_trgm update to "1.

That seems more consistent with other cases, and it's the same
number of <TAB> presses.

            regards, tom lane

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 7f0ab5acb9..941daaeb7c 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1081,19 +1081,10 @@ static const SchemaQuery Query_for_trigger_of_table = {
 "   FROM pg_catalog.pg_available_extensions "\
 "  WHERE name LIKE '%s' AND installed_version IS NULL"

-/* the result of this query is not a raw identifier, so use VERBATIM */
 #define Query_for_list_of_available_extension_versions \
-" SELECT pg_catalog.quote_ident(version) "\
+" SELECT version "\
 "   FROM pg_catalog.pg_available_extension_versions "\
-"  WHERE pg_catalog.quote_ident(version) LIKE '%s'"\
-"    AND name='%s'"
-
-/* the result of this query is not a raw identifier, so use VERBATIM */
-#define Query_for_list_of_available_extension_versions_with_TO \
-" SELECT 'TO ' || pg_catalog.quote_ident(version) "\
-"   FROM pg_catalog.pg_available_extension_versions "\
-"  WHERE ('TO ' || pg_catalog.quote_ident(version)) LIKE '%s'"\
-"    AND name='%s'"
+"  WHERE version LIKE '%s' AND name='%s'"

 #define Query_for_list_of_prepared_statements \
 " SELECT name "\
@@ -1934,20 +1925,17 @@ psql_completion(const char *text, int start, int end)

     /* ALTER EXTENSION <name> */
     else if (Matches("ALTER", "EXTENSION", MatchAny))
-        COMPLETE_WITH("ADD", "DROP", "UPDATE", "SET SCHEMA");
+        COMPLETE_WITH("ADD", "DROP", "UPDATE TO", "SET SCHEMA");

     /* ALTER EXTENSION <name> UPDATE */
     else if (Matches("ALTER", "EXTENSION", MatchAny, "UPDATE"))
-    {
-        set_completion_reference(prev2_wd);
-        COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_available_extension_versions_with_TO);
-    }
+        COMPLETE_WITH("TO");

     /* ALTER EXTENSION <name> UPDATE TO */
     else if (Matches("ALTER", "EXTENSION", MatchAny, "UPDATE", "TO"))
     {
         set_completion_reference(prev3_wd);
-        COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_available_extension_versions);
+        COMPLETE_WITH_QUERY(Query_for_list_of_available_extension_versions);
     }

     /* ALTER FOREIGN */
@@ -2824,7 +2812,7 @@ psql_completion(const char *text, int start, int end)
     else if (Matches("CREATE", "EXTENSION", MatchAny, "VERSION"))
     {
         set_completion_reference(prev2_wd);
-        COMPLETE_WITH_QUERY_VERBATIM(Query_for_list_of_available_extension_versions);
+        COMPLETE_WITH_QUERY(Query_for_list_of_available_extension_versions);
     }

     /* CREATE FOREIGN */

On Sun, Jun 19, 2022 at 12:56:13AM -0400, Tom Lane wrote:
> Actually ... after further thought it seems like maybe we should
> make this more like other cases rather than less so.  ISTM that much
> of the issue here is somebody's decision that "TO version" should be
> offered as a completion of "UPDATE", which is unlike the way we do this
> anywhere else --- the usual thing is to offer "UPDATE TO" as a single
> completion.  So I'm thinking about the attached.

Which are the older completions that offer "UPDATE TO"?  I don't see any.

> This behaves a little differently from the old code.  In v14,
>     alter extension pg_trgm upd<TAB>
> gives you
>     alter extension pg_trgm update<space>
> and another <TAB> produces
>     alter extension pg_trgm update TO "1.
> 
> With this,
>     alter extension pg_trgm upd<TAB>
> gives you
>     alter extension pg_trgm update to<space>
> and another <TAB> produces
>     alter extension pg_trgm update to "1.
> 
> That seems more consistent with other cases, and it's the same
> number of <TAB> presses.

I think it makes sense to send UPDATE TO as a single completion in places
where no valid command can have the UPDATE without the TO.  CREATE RULE foo AS
ON UPDATE TO is a candidate, though CREATE RULE completion doesn't do that
today.  "ALTER EXTENSION hstore UPDATE;" is a valid command (updates to the
control file default version).  Hence, I think the v14 behavior was better.

Noah Misch <noah@leadboat.com> writes:
> I think it makes sense to send UPDATE TO as a single completion in places
> where no valid command can have the UPDATE without the TO.  CREATE RULE foo AS
> ON UPDATE TO is a candidate, though CREATE RULE completion doesn't do that
> today.  "ALTER EXTENSION hstore UPDATE;" is a valid command (updates to the
> control file default version).  Hence, I think the v14 behavior was better.

Hmm ... good point, let me think about that.

            regards, tom lane

I wrote:
> Noah Misch <noah@leadboat.com> writes:
>> "ALTER EXTENSION hstore UPDATE;" is a valid command (updates to the
>> control file default version).  Hence, I think the v14 behavior was better.

> Hmm ... good point, let me think about that.

After consideration, my preferred solution is just this:

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 463cac9fb0..c5cafe6f4b 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1927,7 +1927,7 @@ psql_completion(const char *text, int start, int end)
 
     /* ALTER EXTENSION <name> */
     else if (Matches("ALTER", "EXTENSION", MatchAny))
-        COMPLETE_WITH("ADD", "DROP", "UPDATE TO", "SET SCHEMA");
+        COMPLETE_WITH("ADD", "DROP", "UPDATE", "SET SCHEMA");
 
     /* ALTER EXTENSION <name> UPDATE */
     else if (Matches("ALTER", "EXTENSION", MatchAny, "UPDATE"))

This will require one extra <TAB> when what you want is to update to
a specific version, but I doubt that that's going to bother anyone
very much.  I don't want to try to resurrect the v14 behavior exactly
because it's too much of a mess from a quoting standpoint.

            regards, tom lane

On Sun, Jul 03, 2022 at 02:00:59PM -0400, Tom Lane wrote:
> I wrote:
> > Noah Misch <noah@leadboat.com> writes:
> >> "ALTER EXTENSION hstore UPDATE;" is a valid command (updates to the
> >> control file default version).  Hence, I think the v14 behavior was better.
> 
> > Hmm ... good point, let me think about that.
> 
> After consideration, my preferred solution is just this:
> 
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 463cac9fb0..c5cafe6f4b 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1927,7 +1927,7 @@ psql_completion(const char *text, int start, int end)
>  
>      /* ALTER EXTENSION <name> */
>      else if (Matches("ALTER", "EXTENSION", MatchAny))
> -        COMPLETE_WITH("ADD", "DROP", "UPDATE TO", "SET SCHEMA");
> +        COMPLETE_WITH("ADD", "DROP", "UPDATE", "SET SCHEMA");
>  
>      /* ALTER EXTENSION <name> UPDATE */
>      else if (Matches("ALTER", "EXTENSION", MatchAny, "UPDATE"))
> 
> This will require one extra <TAB> when what you want is to update to
> a specific version, but I doubt that that's going to bother anyone
> very much.  I don't want to try to resurrect the v14 behavior exactly
> because it's too much of a mess from a quoting standpoint.

Works for me, and I agree the patch implements that successfully.  "ALTER
EXTENSION x UPDATE;" is an infrequent command, and "ALTER EXTENSION x UPDATE
TO ..." is even less frequent.  It's not worth much special effort to shave
<TAB> steps.

Noah Misch <noah@leadboat.com> writes:
> On Sun, Jul 03, 2022 at 02:00:59PM -0400, Tom Lane wrote:
>> This will require one extra <TAB> when what you want is to update to
>> a specific version, but I doubt that that's going to bother anyone
>> very much.  I don't want to try to resurrect the v14 behavior exactly
>> because it's too much of a mess from a quoting standpoint.

> Works for me, and I agree the patch implements that successfully.  "ALTER
> EXTENSION x UPDATE;" is an infrequent command, and "ALTER EXTENSION x UPDATE
> TO ..." is even less frequent.  It's not worth much special effort to shave
> <TAB> steps.

Done that way then.  Thanks for the report.

            regards, tom lane