Identifying function-lookup failures due to argument name mismatches - Mailing list pgsql-hackers

We've had various complaints about how our error reports aren't too
helpful if a function lookup failure occurs because you misspell
the name of a named argument.  The most recent is at [1], but there
have been others if memory serves.

I took a swing at improving this, as attached.  It turns out to be
about as messy as I feared, because the basic question of "did the
argument names match" turns out to be intermixed with a bunch of
random rules about default arguments and precisely how you're allowed
to mix named and positional arguments.  So, of the three existing
test cases that this patch changes the results for, the first change
is quite on-point but the second and third maybe not so much.

Perhaps this could be improved further with some refactoring, but the
function lookup logic is complicated and changing it would raise the
odds of introducing a bug quite a lot.

Another thing not to like is that it seems like this is doing violence
to several APIs in exchange for not very much improvement in the error
messages.  I feel like maybe we ought to be trying for more
specificity about additional cases, but I'm not very sure what else
to improve or what the API could look like.

Anyway, I'm not seriously proposing that this should be committed
as-is.  I'm throwing it out there in case anyone else has a good
idea or feels motivated to push on the problem some more.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CAFCRh-_iLoUtMAtyunw_-O6sgpWo04sOmB38MUVNpuQVSkL_0Q%40mail.gmail.com

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index d97d632a7ef..100c58b658a 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -1184,17 +1184,24 @@ TypeIsVisibleExt(Oid typid, bool *is_missing)
  * The caller might end up discarding such an entry anyway, but if it selects
  * such an entry it should react as though the call were ambiguous.
  *
- * If missing_ok is true, an empty list (NULL) is returned if the name was
- * schema-qualified with a schema that does not exist.  Likewise if no
- * candidate is found for other reasons.
+ * We return an empty list (NULL) if no suitable matches can be found.
+ * *bad_argnames is set true if there were matches to the function name
+ * but not to the argnames, or to false in all other cases.
+ *
+ * If the function name was schema-qualified with a schema that does not
+ * exist, then we return an empty list if missing_ok is true and otherwise
+ * throw an error.  (missing_ok does not affect the behavior otherwise.)
  */
 FuncCandidateList
 FuncnameGetCandidates(List *names, int nargs, List *argnames,
                       bool expand_variadic, bool expand_defaults,
-                      bool include_out_arguments, bool missing_ok)
+                      bool include_out_arguments, bool missing_ok,
+                      bool *bad_argnames)
 {
     FuncCandidateList resultList = NULL;
     bool        any_special = false;
+    bool        found_name_match = false;
+    bool        found_argnames_match = false;
     char       *schemaname;
     char       *funcname;
     Oid            namespaceId;
@@ -1204,6 +1211,9 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
     /* check for caller error */
     Assert(nargs >= 0 || !(expand_variadic | expand_defaults));

+    /* set default output */
+    *bad_argnames = false;
+
     /* deconstruct the name list */
     DeconstructQualifiedName(names, &schemaname, &funcname);

@@ -1263,6 +1273,9 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
                 continue;        /* proc is not in search path */
         }

+        /* Okay, we have a name (and schema) match */
+        found_name_match = true;
+
         /*
          * If we are asked to match to OUT arguments, then use the
          * proallargtypes array (which includes those); otherwise use
@@ -1331,6 +1344,9 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
                                 &argnumbers))
                 continue;

+            /* We have found at least one match to the argument names */
+            found_argnames_match = true;
+
             /* Named argument matching is always "special" */
             any_special = true;
         }
@@ -1557,6 +1573,14 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,

     ReleaseSysCacheList(catlist);

+    /*
+     * Detect case where there were possible matches but they all failed to
+     * match the given argnames.
+     */
+    if (resultList == NULL && argnames != NIL &&
+        found_name_match && !found_argnames_match)
+        *bad_argnames = true;
+
     return resultList;
 }

@@ -1746,11 +1770,13 @@ FunctionIsVisibleExt(Oid funcid, bool *is_missing)
         char       *proname = NameStr(procform->proname);
         int            nargs = procform->pronargs;
         FuncCandidateList clist;
+        bool        bad_argnames;

         visible = false;

         clist = FuncnameGetCandidates(list_make1(makeString(proname)),
-                                      nargs, NIL, false, false, false, false);
+                                      nargs, NIL, false, false, false, false,
+                                      &bad_argnames);

         for (; clist; clist = clist->next)
         {
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 583bbbf232f..e8383ef99d7 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -601,9 +601,30 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,

         /*
          * No function, and no column either.  Since we're dealing with
-         * function notation, report "function does not exist".
+         * function notation, report "function/procedure does not exist"
+         * rather than mentioning columns.  We have a couple of different
+         * things to say as detail or hint, though.
          */
-        if (list_length(agg_order) > 1 && !agg_within_group)
+        if (fdresult == FUNCDETAIL_BAD_ARGNAMES)
+        {
+            if (proc_call)
+                ereport(ERROR,
+                        (errcode(ERRCODE_UNDEFINED_FUNCTION),
+                         errmsg("procedure %s does not exist",
+                                func_signature_string(funcname, nargs, argnames,
+                                                      actual_arg_types)),
+                         errdetail("No procedure matches the given argument name(s)."),
+                         parser_errposition(pstate, location)));
+            else
+                ereport(ERROR,
+                        (errcode(ERRCODE_UNDEFINED_FUNCTION),
+                         errmsg("function %s does not exist",
+                                func_signature_string(funcname, nargs, argnames,
+                                                      actual_arg_types)),
+                         errdetail("No function matches the given argument name(s)."),
+                         parser_errposition(pstate, location)));
+        }
+        else if (list_length(agg_order) > 1 && !agg_within_group)
         {
             /* It's agg(x, ORDER BY y,z) ... perhaps misplaced ORDER BY */
             ereport(ERROR,
@@ -1410,6 +1431,7 @@ func_get_detail(List *funcname,
 {
     FuncCandidateList raw_candidates;
     FuncCandidateList best_candidate;
+    bool        bad_argnames;

     /* initialize output arguments to silence compiler warnings */
     *funcid = InvalidOid;
@@ -1424,7 +1446,8 @@ func_get_detail(List *funcname,
     /* Get list of possible candidates from namespace search */
     raw_candidates = FuncnameGetCandidates(funcname, nargs, fargnames,
                                            expand_variadic, expand_defaults,
-                                           include_out_arguments, false);
+                                           include_out_arguments, false,
+                                           &bad_argnames);

     /*
      * Quickly check if there is an exact match to the input datatypes (there
@@ -1594,7 +1617,7 @@ func_get_detail(List *funcname,
          */
         if (fargnames != NIL && !expand_variadic && nargs > 0 &&
             best_candidate->argnumbers[nargs - 1] != nargs - 1)
-            return FUNCDETAIL_NOTFOUND;
+            return FUNCDETAIL_BAD_ARGNAMES;

         *funcid = best_candidate->oid;
         *nvargs = best_candidate->nvargs;
@@ -1719,6 +1742,12 @@ func_get_detail(List *funcname,
         return result;
     }

+    /*
+     * For error reporting purposes, we want to distinguish the case of
+     * could-not-find-an-argnames-match from other cases.
+     */
+    if (bad_argnames)
+        return FUNCDETAIL_BAD_ARGNAMES;
     return FUNCDETAIL_NOTFOUND;
 }

@@ -2053,6 +2082,7 @@ LookupFuncNameInternal(ObjectType objtype, List *funcname,
 {
     Oid            result = InvalidOid;
     FuncCandidateList clist;
+    bool        bad_argnames;

     /* NULL argtypes allowed for nullary functions only */
     Assert(argtypes != NULL || nargs == 0);
@@ -2062,7 +2092,8 @@ LookupFuncNameInternal(ObjectType objtype, List *funcname,

     /* Get list of candidate objects */
     clist = FuncnameGetCandidates(funcname, nargs, NIL, false, false,
-                                  include_out_arguments, missing_ok);
+                                  include_out_arguments, missing_ok,
+                                  &bad_argnames);

     /* Scan list for a match to the arg types (if specified) and the objtype */
     for (; clist != NULL; clist = clist->next)
diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index b8bbe95e82e..9b70305bfc6 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -71,6 +71,7 @@ regprocin(PG_FUNCTION_ARGS)
     RegProcedure result;
     List       *names;
     FuncCandidateList clist;
+    bool        bad_argnames;

     /* Handle "-" or numeric OID */
     if (parseDashOrOid(pro_name_or_oid, &result, escontext))
@@ -93,7 +94,8 @@ regprocin(PG_FUNCTION_ARGS)
     if (names == NIL)
         PG_RETURN_NULL();

-    clist = FuncnameGetCandidates(names, -1, NIL, false, false, false, true);
+    clist = FuncnameGetCandidates(names, -1, NIL, false, false, false, true,
+                                  &bad_argnames);

     if (clist == NULL)
         ereturn(escontext, (Datum) 0,
@@ -164,13 +166,15 @@ regprocout(PG_FUNCTION_ARGS)
         {
             char       *nspname;
             FuncCandidateList clist;
+            bool        bad_argnames;

             /*
              * Would this proc be found (uniquely!) by regprocin? If not,
              * qualify it.
              */
             clist = FuncnameGetCandidates(list_make1(makeString(proname)),
-                                          -1, NIL, false, false, false, false);
+                                          -1, NIL, false, false, false, false,
+                                          &bad_argnames);
             if (clist != NULL && clist->next == NULL &&
                 clist->oid == proid)
                 nspname = NULL;
@@ -231,6 +235,7 @@ regprocedurein(PG_FUNCTION_ARGS)
     int            nargs;
     Oid            argtypes[FUNC_MAX_ARGS];
     FuncCandidateList clist;
+    bool        bad_argnames;

     /* Handle "-" or numeric OID */
     if (parseDashOrOid(pro_name_or_oid, &result, escontext))
@@ -252,7 +257,8 @@ regprocedurein(PG_FUNCTION_ARGS)
         PG_RETURN_NULL();

     clist = FuncnameGetCandidates(names, nargs, NIL, false, false,
-                                  false, true);
+                                  false, true,
+                                  &bad_argnames);

     for (; clist; clist = clist->next)
     {
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 8c7ccc69a3c..9ab5fb5daf6 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -102,7 +102,8 @@ extern FuncCandidateList FuncnameGetCandidates(List *names,
                                                bool expand_variadic,
                                                bool expand_defaults,
                                                bool include_out_arguments,
-                                               bool missing_ok);
+                                               bool missing_ok,
+                                               bool *bad_argnames);
 extern bool FunctionIsVisible(Oid funcid);

 extern Oid    OpernameGetOprid(List *names, Oid oprleft, Oid oprright);
diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h
index a6f24b83d84..29af0f58ec0 100644
--- a/src/include/parser/parse_func.h
+++ b/src/include/parser/parse_func.h
@@ -22,6 +22,7 @@
 typedef enum
 {
     FUNCDETAIL_NOTFOUND,        /* no matching function */
+    FUNCDETAIL_BAD_ARGNAMES,    /* no matching function because of arg names */
     FUNCDETAIL_MULTIPLE,        /* too many matching functions */
     FUNCDETAIL_NORMAL,            /* found a matching regular function */
     FUNCDETAIL_PROCEDURE,        /* found a matching procedure */
diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 94eedfe375e..83576c739b5 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -1448,17 +1448,17 @@ select * from dfunc(x := 10, b := 20, c := 30);  -- fail, unknown param
 ERROR:  function dfunc(x => integer, b => integer, c => integer) does not exist
 LINE 1: select * from dfunc(x := 10, b := 20, c := 30);
                       ^
-HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
+DETAIL:  No function matches the given argument name(s).
 select * from dfunc(10, 10, a := 20);  -- fail, a overlaps positional parameter
 ERROR:  function dfunc(integer, integer, a => integer) does not exist
 LINE 1: select * from dfunc(10, 10, a := 20);
                       ^
-HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
+DETAIL:  No function matches the given argument name(s).
 select * from dfunc(1,c := 2,d := 3); -- fail, no value for b
 ERROR:  function dfunc(integer, c => integer, d => integer) does not exist
 LINE 1: select * from dfunc(1,c := 2,d := 3);
                       ^
-HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
+DETAIL:  No function matches the given argument name(s).
 drop function dfunc(int, int, int, int);
 -- test with different parameter types
 create function dfunc(a varchar, b numeric, c date = current_date)