Tab completion of function arguments not working in all cases
Hi,
I noticed this while testing 9.2, but it seems to go back to at least
8.3. Tab completion of function arguments doesn't work if the function
is schema-qualified or double-quoted. So for example,
DROP FUNCTION my_function ( <TAB>
completes the functions arguments, but
DROP FUNCTION my_schema.my_function ( <TAB>
doesn't offer any completions, and nor does
DROP FUNCTION "my function" ( <TAB>
The attached patch fixes these problems by introducing a new macro
COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
seems to be the nearest analogous code that covers all the edge cases.
Regards,
Dean
Attachments:
tab-complete.patchapplication/octet-stream; name=tab-complete.patchDownload
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index a50e735..e852349
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** do { \
*** 202,207 ****
--- 202,232 ----
matches = completion_matches(text, complete_from_query); \
} while (0)
+ #define COMPLETE_WITH_ARG(function) \
+ do { \
+ char *_completion_schema; \
+ char *_completion_function; \
+ \
+ _completion_schema = strtokx(function, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ (void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ _completion_function = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ if (_completion_function == NULL) \
+ { \
+ completion_charp = Query_for_list_of_arguments; \
+ completion_info_charp = function; \
+ } \
+ else \
+ { \
+ completion_charp = Query_for_list_of_arguments_with_schema; \
+ completion_info_charp = _completion_function; \
+ completion_info_charp2 = _completion_schema; \
+ } \
+ matches = completion_matches(text, complete_from_query); \
+ } while (0)
+
/*
* Assembly instructions for schema queries
*/
*************** static const SchemaQuery Query_for_list_
*** 598,607 ****
" FROM pg_catalog.pg_am "\
" WHERE substring(pg_catalog.quote_ident(amname),1,%d)='%s'"
#define Query_for_list_of_arguments \
! " SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc "\
! " WHERE proname='%s'"
#define Query_for_list_of_extensions \
" SELECT pg_catalog.quote_ident(extname) "\
--- 623,646 ----
" FROM pg_catalog.pg_am "\
" WHERE substring(pg_catalog.quote_ident(amname),1,%d)='%s'"
+ /* the silly-looking length condition is just to eat up the current word */
#define Query_for_list_of_arguments \
! "SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc "\
! " WHERE (%d = pg_catalog.length('%s'))"\
! " AND (pg_catalog.quote_ident(proname)='%s'"\
! " OR '\"' || proname || '\"'='%s') "
!
! /* the silly-looking length condition is just to eat up the current word */
! #define Query_for_list_of_arguments_with_schema \
! "SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc p, pg_catalog.pg_namespace n "\
! " WHERE (%d = pg_catalog.length('%s'))"\
! " AND n.oid = p.pronamespace "\
! " AND (pg_catalog.quote_ident(proname)='%s' "\
! " OR '\"' || proname || '\"' ='%s') "\
! " AND (pg_catalog.quote_ident(nspname)='%s' "\
! " OR '\"' || nspname || '\"' ='%s') "
#define Query_for_list_of_extensions \
" SELECT pg_catalog.quote_ident(extname) "\
*************** psql_completion(char *text, int start, i
*** 863,875 ****
COMPLETE_WITH_LIST(list_ALTERAGG);
}
else
! {
! char *tmp_buf = malloc(strlen(Query_for_list_of_arguments) + strlen(prev2_wd));
!
! sprintf(tmp_buf, Query_for_list_of_arguments, prev2_wd);
! COMPLETE_WITH_QUERY(tmp_buf);
! free(tmp_buf);
! }
}
/* ALTER SCHEMA <name> */
--- 902,908 ----
COMPLETE_WITH_LIST(list_ALTERAGG);
}
else
! COMPLETE_WITH_ARG(prev2_wd);
}
/* ALTER SCHEMA <name> */
*************** psql_completion(char *text, int start, i
*** 2186,2198 ****
(pg_strcasecmp(prev3_wd, "AGGREGATE") == 0 ||
pg_strcasecmp(prev3_wd, "FUNCTION") == 0) &&
pg_strcasecmp(prev_wd, "(") == 0)
! {
! char *tmp_buf = malloc(strlen(Query_for_list_of_arguments) + strlen(prev2_wd));
!
! sprintf(tmp_buf, Query_for_list_of_arguments, prev2_wd);
! COMPLETE_WITH_QUERY(tmp_buf);
! free(tmp_buf);
! }
/* DROP OWNED BY */
else if (pg_strcasecmp(prev2_wd, "DROP") == 0 &&
pg_strcasecmp(prev_wd, "OWNED") == 0)
--- 2219,2225 ----
(pg_strcasecmp(prev3_wd, "AGGREGATE") == 0 ||
pg_strcasecmp(prev3_wd, "FUNCTION") == 0) &&
pg_strcasecmp(prev_wd, "(") == 0)
! COMPLETE_WITH_ARG(prev2_wd);
/* DROP OWNED BY */
else if (pg_strcasecmp(prev2_wd, "DROP") == 0 &&
pg_strcasecmp(prev_wd, "OWNED") == 0)
On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
I noticed this while testing 9.2, but it seems to go back to at least
8.3. Tab completion of function arguments doesn't work if the function
is schema-qualified or double-quoted. So for example,
Good idea, would you like to submit for the open CF?
Josh
On 12 June 2012 21:27, Josh Kupershmidt <schmiddy@gmail.com> wrote:
On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
I noticed this while testing 9.2, but it seems to go back to at least
8.3. Tab completion of function arguments doesn't work if the function
is schema-qualified or double-quoted. So for example,Good idea, would you like to submit for the open CF?
Hmm, I was thinking of this as a bug because I found it during beta
testing, but on reflection I guess that it's more of an annoying
limitation than a bug per se. Also since no one appears to have
noticed until now, I guess it's not worth back-patching, so I'll
submit it for 9.3.
Regards,
Dean
[Hope it's OK if I move this thread to -hackers, as part of CF review.]
On Sat, Jun 9, 2012 at 2:40 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Hi,
I noticed this while testing 9.2, but it seems to go back to at least
8.3. Tab completion of function arguments doesn't work if the function
is schema-qualified or double-quoted. So for example,DROP FUNCTION my_function ( <TAB>
completes the functions arguments, but
DROP FUNCTION my_schema.my_function ( <TAB>
doesn't offer any completions, and nor does
DROP FUNCTION "my function" ( <TAB>
+1 for the idea. I find the existing behavior rather confusing,
particularly the fact that a schema-qualified function name will be
tab-completed, i.e. this works.
DROP FUNCTION my_schema.my<TAB>
but then, as your second example above shows, no completions are
subsequently offered for the function arguments.
As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
DROP FUNCTION my_f<TAB>
which completes to:
DROP FUNCTION my_function
enter parenthesis, and hit tab:
DROP FUNCTION my_function(<TAB>
which, if there is only one match, could complete to:
DROP FUNCTION my_function(integer)
when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.
The attached patch fixes these problems by introducing a new macro
COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
seems to be the nearest analogous code that covers all the edge cases.
Anyway, on to the review of the patch itself:
* Submission *
Patch applies cleanly to git head, and regression tests are not
expected for tab-completion enhancements.
* Features & Usability *
I've verified that tab-completing of the first argument to functions
for DROP FUNCTION and ALTER FUNCTION commands for the most part works
as expected. The one catch I noticed was that
Query_for_list_of_arguments wasn't restricting its results to
currently-visible functions, so with a default search_path, if you
have these two functions defined:
public.doppelganger(text)
my_schema.doppelganger(bytea)
and then try:
DROP FUNCTION doppelganger(<TAB>
you get tab-completions for both "text)" and "bytea(", when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.
* Coding *
The new macro COMPLETE_WITH_ARG seems fine. The existing code used
malloc() directly for DROP FUNCTION and ALTER FUNCTION
(tab-complete.c, around lines 867 and 2190), which AIUI is frowned
upon in favor of pg_malloc(). The patch avoids this ugliness by using
the new COMPLETE_WITH_ARG macro, so that's a nice fixup.
Overall, a nice fix for an overlooked piece of the tab-completion machinery.
Josh
Attachments:
tab-complete.funcargs.v2.diffapplication/octet-stream; name=tab-complete.funcargs.v2.diffDownload
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 061acd1..f263103
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** do { \
*** 202,207 ****
--- 202,232 ----
matches = completion_matches(text, complete_from_query); \
} while (0)
+ #define COMPLETE_WITH_ARG(function) \
+ do { \
+ char *_completion_schema; \
+ char *_completion_function; \
+ \
+ _completion_schema = strtokx(function, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ (void) strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ _completion_function = strtokx(NULL, " \t\n\r", ".", "\"", 0, \
+ false, false, pset.encoding); \
+ if (_completion_function == NULL) \
+ { \
+ completion_charp = Query_for_list_of_arguments; \
+ completion_info_charp = function; \
+ } \
+ else \
+ { \
+ completion_charp = Query_for_list_of_arguments_with_schema; \
+ completion_info_charp = _completion_function; \
+ completion_info_charp2 = _completion_schema; \
+ } \
+ matches = completion_matches(text, complete_from_query); \
+ } while (0)
+
/*
* Assembly instructions for schema queries
*/
*************** static const SchemaQuery Query_for_list_
*** 598,607 ****
" FROM pg_catalog.pg_am "\
" WHERE substring(pg_catalog.quote_ident(amname),1,%d)='%s'"
#define Query_for_list_of_arguments \
! " SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc "\
! " WHERE proname='%s'"
#define Query_for_list_of_extensions \
" SELECT pg_catalog.quote_ident(extname) "\
--- 623,647 ----
" FROM pg_catalog.pg_am "\
" WHERE substring(pg_catalog.quote_ident(amname),1,%d)='%s'"
+ /* the silly-looking length condition is just to eat up the current word */
#define Query_for_list_of_arguments \
! "SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc "\
! " WHERE (%d = pg_catalog.length('%s'))"\
! " AND (pg_catalog.quote_ident(proname)='%s'"\
! " OR '\"' || proname || '\"'='%s') "\
! " AND (pg_catalog.pg_function_is_visible(pg_proc.oid))"
!
! /* the silly-looking length condition is just to eat up the current word */
! #define Query_for_list_of_arguments_with_schema \
! "SELECT pg_catalog.oidvectortypes(proargtypes)||')' "\
! " FROM pg_catalog.pg_proc p, pg_catalog.pg_namespace n "\
! " WHERE (%d = pg_catalog.length('%s'))"\
! " AND n.oid = p.pronamespace "\
! " AND (pg_catalog.quote_ident(proname)='%s' "\
! " OR '\"' || proname || '\"' ='%s') "\
! " AND (pg_catalog.quote_ident(nspname)='%s' "\
! " OR '\"' || nspname || '\"' ='%s') "
#define Query_for_list_of_extensions \
" SELECT pg_catalog.quote_ident(extname) "\
*************** psql_completion(char *text, int start, i
*** 863,875 ****
COMPLETE_WITH_LIST(list_ALTERAGG);
}
else
! {
! char *tmp_buf = malloc(strlen(Query_for_list_of_arguments) + strlen(prev2_wd));
!
! sprintf(tmp_buf, Query_for_list_of_arguments, prev2_wd);
! COMPLETE_WITH_QUERY(tmp_buf);
! free(tmp_buf);
! }
}
/* ALTER SCHEMA <name> */
--- 903,909 ----
COMPLETE_WITH_LIST(list_ALTERAGG);
}
else
! COMPLETE_WITH_ARG(prev2_wd);
}
/* ALTER SCHEMA <name> */
*************** psql_completion(char *text, int start, i
*** 2186,2198 ****
(pg_strcasecmp(prev3_wd, "AGGREGATE") == 0 ||
pg_strcasecmp(prev3_wd, "FUNCTION") == 0) &&
pg_strcasecmp(prev_wd, "(") == 0)
! {
! char *tmp_buf = malloc(strlen(Query_for_list_of_arguments) + strlen(prev2_wd));
!
! sprintf(tmp_buf, Query_for_list_of_arguments, prev2_wd);
! COMPLETE_WITH_QUERY(tmp_buf);
! free(tmp_buf);
! }
/* DROP OWNED BY */
else if (pg_strcasecmp(prev2_wd, "DROP") == 0 &&
pg_strcasecmp(prev_wd, "OWNED") == 0)
--- 2220,2226 ----
(pg_strcasecmp(prev3_wd, "AGGREGATE") == 0 ||
pg_strcasecmp(prev3_wd, "FUNCTION") == 0) &&
pg_strcasecmp(prev_wd, "(") == 0)
! COMPLETE_WITH_ARG(prev2_wd);
/* DROP OWNED BY */
else if (pg_strcasecmp(prev2_wd, "DROP") == 0 &&
pg_strcasecmp(prev_wd, "OWNED") == 0)
On 18 June 2012 04:21, Josh Kupershmidt <schmiddy@gmail.com> wrote:
+1 for the idea. I find the existing behavior rather confusing,
particularly the fact that a schema-qualified function name will be
tab-completed, i.e. this works.DROP FUNCTION my_schema.my<TAB>
but then, as your second example above shows, no completions are
subsequently offered for the function arguments.As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
DROP FUNCTION my_f<TAB>which completes to:
DROP FUNCTION my_functionenter parenthesis, and hit tab:
DROP FUNCTION my_function(<TAB>which, if there is only one match, could complete to:
DROP FUNCTION my_function(integer)when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.
Hmm, I find that it does automatically fill in the opening
parenthesis, but only once there is a space after the completed
function name. So
"DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
(note the space at the end). Then pressing <TAB> one more time gives
"DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
the function arguments.
Alternatively "DROP FUNCTION my_function<TAB>" (no space after
function name) first completes to "DROP FUNCTION my_function " (adding
the space), and then completes with the opening parenthesis, and then
with the function arguments.
It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
than typing the opening bracket.
The attached patch fixes these problems by introducing a new macro
COMPLETE_WITH_ARG, similar to the existing COMPLETE_WITH_ATTR, which
seems to be the nearest analogous code that covers all the edge cases.Anyway, on to the review of the patch itself:
* Submission *
Patch applies cleanly to git head, and regression tests are not
expected for tab-completion enhancements.* Features & Usability *
I've verified that tab-completing of the first argument to functions
for DROP FUNCTION and ALTER FUNCTION commands for the most part works
as expected. The one catch I noticed was that
Query_for_list_of_arguments wasn't restricting its results to
currently-visible functions, so with a default search_path, if you
have these two functions defined:public.doppelganger(text)
my_schema.doppelganger(bytea)and then try:
DROP FUNCTION doppelganger(<TAB>
you get tab-completions for both "text)" and "bytea(", when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.
Good catch.
I think that's a useful additional test, and is also consistent with
the existing code in Query_for_list_of_attributes.
* Coding *
The new macro COMPLETE_WITH_ARG seems fine. The existing code used
malloc() directly for DROP FUNCTION and ALTER FUNCTION
(tab-complete.c, around lines 867 and 2190), which AIUI is frowned
upon in favor of pg_malloc(). The patch avoids this ugliness by using
the new COMPLETE_WITH_ARG macro, so that's a nice fixup.Overall, a nice fix for an overlooked piece of the tab-completion machinery.
Josh
Thanks for the review.
Cheers,
Dean
On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 18 June 2012 04:21, Josh Kupershmidt <schmiddy@gmail.com> wrote:
As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
DROP FUNCTION my_f<TAB>which completes to:
DROP FUNCTION my_functionenter parenthesis, and hit tab:
DROP FUNCTION my_function(<TAB>which, if there is only one match, could complete to:
DROP FUNCTION my_function(integer)when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.Hmm, I find that it does automatically fill in the opening
parenthesis, but only once there is a space after the completed
function name. So
"DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
(note the space at the end). Then pressing <TAB> one more time gives
"DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
the function arguments.Alternatively "DROP FUNCTION my_function<TAB>" (no space after
function name) first completes to "DROP FUNCTION my_function " (adding
the space), and then completes with the opening parenthesis, and then
with the function arguments.It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
than typing the opening bracket.
Interesting, I see the behavior you describe on Linux, using psql +
libreadline, and the behavior I showed (i.e. repeatedly pressing tab
won't automatically fill in the function arguments after the function
name is completed, seemingly because no space is deposited after the
completed function name) is with OS X 10.6, using psql + libedit.
[snip]
you get tab-completions for both "text)" and "bytea(", when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.Good catch.
I think that's a useful additional test, and is also consistent with
the existing code in Query_for_list_of_attributes.
OK, I'll mark Ready for Committer in the CF.
Josh
On Mon, Jun 18, 2012 at 5:45 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
On Mon, Jun 18, 2012 at 3:56 AM, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On 18 June 2012 04:21, Josh Kupershmidt <schmiddy@gmail.com> wrote:
As a side note unrelated to this patch, I also dislike how function
name tab-completions will not fill in the opening parenthesis, which
makes for unnecessary work for the user, as one then has to type the
parenthesis and hit tab again to get possible completions for the
function arguments. The current behavior causes:
DROP FUNCTION my_f<TAB>which completes to:
DROP FUNCTION my_functionenter parenthesis, and hit tab:
DROP FUNCTION my_function(<TAB>which, if there is only one match, could complete to:
DROP FUNCTION my_function(integer)when the last three steps could have been consolidated with better
tab-completion. Perhaps this could be a TODO.Hmm, I find that it does automatically fill in the opening
parenthesis, but only once there is a space after the completed
function name. So
"DROP FUNCTION my_f<TAB>" completes to "DROP FUNCTION my_function "
(note the space at the end). Then pressing <TAB> one more time gives
"DROP FUNCTION my_function ( ", and then pressing <TAB> again gives
the function arguments.Alternatively "DROP FUNCTION my_function<TAB>" (no space after
function name) first completes to "DROP FUNCTION my_function " (adding
the space), and then completes with the opening parenthesis, and then
with the function arguments.It's a bit clunky, but I find that repeatedly pressing <TAB> is easier
than typing the opening bracket.Interesting, I see the behavior you describe on Linux, using psql +
libreadline, and the behavior I showed (i.e. repeatedly pressing tab
won't automatically fill in the function arguments after the function
name is completed, seemingly because no space is deposited after the
completed function name) is with OS X 10.6, using psql + libedit.[snip]
you get tab-completions for both "text)" and "bytea(", when you
probably expected only the former. That's easy to fix though, please
see attached v2 patch.Good catch.
I think that's a useful additional test, and is also consistent with
the existing code in Query_for_list_of_attributes.OK, I'll mark Ready for Committer in the CF.
Applied with minor revisions:
* the comment above the COMPLETE_WITH_xyz macros needed an update
* I renamed the macro to COMPLETE_WITH_FUNCTION_ARG - to make it even
more clear what it does
Thanks!
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/