Provide more-specific error details/hints for function lookup failures.

Up to now we've contented ourselves with a one-size-fits-all error
hint when we fail to find any match to a function or procedure call.
That was mostly okay in the beginning, but it was never great, and
since the introduction of named arguments it's really not adequate.
We at least ought to distinguish "function name doesn't exist" from
"function name exists, but not with those argument names".  And the
rules for named-argument matching are arcane enough that some more
detail seems warranted if we match the argument names but the call
still doesn't work.

This patch creates a framework for dealing with these problems:
FuncnameGetCandidates and related code will now pass back a bitmask of
flags showing how far the match succeeded.  This allows a considerable
amount of granularity in the reports.  The set-bits-in-a-bitmask
approach means that when there are multiple candidate functions, the
report will reflect the match(es) that got the furthest, which seems
correct.  Also, we can avoid mentioning "maybe add casts" unless
failure to match argument types is actually the issue.

Extend the same return-a-bitmask approach to OpernameGetCandidates.
The issues around argument names don't apply to operator syntax,
but it still seems worth distinguishing between "there is no
operator of that name" and "we couldn't match the argument types".

While at it, adjust these messages and related ones to more strictly
separate "detail" from "hint", following our message style guidelines'
distinction between those.

Reported-by: Dominique Devienne <ddevienne@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Reviewed-by: Robert Haas <robertmhaas@gmail.com>
Discussion: https://postgr.es/m/1756041.1754616558@sss.pgh.pa.us

1 files changed, 93 insertions, 25 deletions

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 8bd4d6c3d43..ed9aeee24bc 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -232,7 +232,7 @@ static void RemoveTempRelationsCallback(int code, Datum arg);
 static void InvalidationCallback(Datum arg, int cacheid, uint32 hashvalue);
 static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 						   bool include_out_arguments, int pronargs,
-						   int **argnumbers);
+						   int **argnumbers, int *fgc_flags);
 
 /*
  * Recomputing the namespace path can be costly when done frequently, such as
@@ -1117,15 +1117,15 @@ TypeIsVisibleExt(Oid typid, bool *is_missing)
 
 /*
  * FuncnameGetCandidates
- *		Given a possibly-qualified function name and argument count,
+ *		Given a possibly-qualified routine name, argument count, and arg names,
  *		retrieve a list of the possible matches.
  *
- * If nargs is -1, we return all functions matching the given name,
+ * If nargs is -1, we return all routines matching the given name,
  * regardless of argument count.  (argnames must be NIL, and expand_variadic
  * and expand_defaults must be false, in this case.)
  *
  * If argnames isn't NIL, we are considering a named- or mixed-notation call,
- * and only functions having all the listed argument names will be returned.
+ * and only routines having all the listed argument names will be returned.
  * (We assume that length(argnames) <= nargs and all the passed-in names are
  * distinct.)  The returned structs will include an argnumbers array showing
  * the actual argument index for each logical argument position.
@@ -1183,14 +1183,21 @@ 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.
+ * 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.)
+ *
+ * The output argument *fgc_flags is filled with a bitmask indicating how
+ * far we were able to match the supplied information.  This is not of much
+ * interest if any candidates were found, but if not, it can help callers
+ * produce an on-point error message.
  */
 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,
+					  int *fgc_flags)
 {
 	FuncCandidateList resultList = NULL;
 	bool		any_special = false;
@@ -1203,15 +1210,20 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 	/* check for caller error */
 	Assert(nargs >= 0 || !(expand_variadic | expand_defaults));
 
+	/* initialize output fgc_flags to empty */
+	*fgc_flags = 0;
+
 	/* deconstruct the name list */
 	DeconstructQualifiedName(names, &schemaname, &funcname);
 
 	if (schemaname)
 	{
 		/* use exact schema given */
+		*fgc_flags |= FGC_SCHEMA_GIVEN; /* report that a schema is given */
 		namespaceId = LookupExplicitNamespace(schemaname, missing_ok);
 		if (!OidIsValid(namespaceId))
 			return NULL;
+		*fgc_flags |= FGC_SCHEMA_EXISTS;	/* report that the schema exists */
 	}
 	else
 	{
@@ -1237,6 +1249,8 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 		int		   *argnumbers = NULL;
 		FuncCandidateList newResult;
 
+		*fgc_flags |= FGC_NAME_EXISTS;	/* the name is present in pg_proc */
+
 		if (OidIsValid(namespaceId))
 		{
 			/* Consider only procs in specified namespace */
@@ -1262,6 +1276,8 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 				continue;		/* proc is not in search path */
 		}
 
+		*fgc_flags |= FGC_NAME_VISIBLE; /* routine is in the right schema */
+
 		/*
 		 * If we are asked to match to OUT arguments, then use the
 		 * proallargtypes array (which includes those); otherwise use
@@ -1296,16 +1312,6 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 			/*
 			 * Call uses named or mixed notation
 			 *
-			 * Named or mixed notation can match a variadic function only if
-			 * expand_variadic is off; otherwise there is no way to match the
-			 * presumed-nameless parameters expanded from the variadic array.
-			 */
-			if (OidIsValid(procform->provariadic) && expand_variadic)
-				continue;
-			va_elem_type = InvalidOid;
-			variadic = false;
-
-			/*
 			 * Check argument count.
 			 */
 			Assert(nargs >= 0); /* -1 not supported with argnames */
@@ -1324,11 +1330,32 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 			if (pronargs != nargs && !use_defaults)
 				continue;
 
+			/* We found a routine with a suitable number of arguments */
+			*fgc_flags |= FGC_ARGCOUNT_MATCH;
+
 			/* Check for argument name match, generate positional mapping */
 			if (!MatchNamedCall(proctup, nargs, argnames,
 								include_out_arguments, pronargs,
-								&argnumbers))
+								&argnumbers, fgc_flags))
+				continue;
+
+			/*
+			 * Named or mixed notation can match a variadic function only if
+			 * expand_variadic is off; otherwise there is no way to match the
+			 * presumed-nameless parameters expanded from the variadic array.
+			 * However, we postpone the check until here because we want to
+			 * perform argument name matching anyway (using the variadic array
+			 * argument's name).  This allows us to give an on-point error
+			 * message if the user forgets to say VARIADIC in what would have
+			 * been a valid call with it.
+			 */
+			if (OidIsValid(procform->provariadic) && expand_variadic)
 				continue;
+			va_elem_type = InvalidOid;
+			variadic = false;
+
+			/* We found a fully-valid call using argument names */
+			*fgc_flags |= FGC_ARGNAMES_VALID;
 
 			/* Named argument matching is always "special" */
 			any_special = true;
@@ -1371,6 +1398,9 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
 			/* Ignore if it doesn't match requested argument count */
 			if (nargs >= 0 && pronargs != nargs && !variadic && !use_defaults)
 				continue;
+
+			/* We found a routine with a suitable number of arguments */
+			*fgc_flags |= FGC_ARGCOUNT_MATCH;
 		}
 
 		/*
@@ -1579,11 +1609,13 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
  * the mapping from call argument positions to actual function argument
  * numbers.  Defaulted arguments are included in this map, at positions
  * after the last supplied argument.
+ *
+ * We also add flag bits to *fgc_flags reporting on how far the match got.
  */
 static bool
 MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 			   bool include_out_arguments, int pronargs,
-			   int **argnumbers)
+			   int **argnumbers, int *fgc_flags)
 {
 	Form_pg_proc procform = (Form_pg_proc) GETSTRUCT(proctup);
 	int			numposargs = nargs - list_length(argnames);
@@ -1592,6 +1624,7 @@ MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 	char	  **p_argnames;
 	char	   *p_argmodes;
 	bool		arggiven[FUNC_MAX_ARGS];
+	bool		arg_filled_twice = false;
 	bool		isnull;
 	int			ap;				/* call args position */
 	int			pp;				/* proargs position */
@@ -1645,9 +1678,9 @@ MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 				continue;
 			if (p_argnames[i] && strcmp(p_argnames[i], argname) == 0)
 			{
-				/* fail if argname matches a positional argument */
+				/* note if argname matches a positional argument */
 				if (arggiven[pp])
-					return false;
+					arg_filled_twice = true;
 				arggiven[pp] = true;
 				(*argnumbers)[ap] = pp;
 				found = true;
@@ -1664,6 +1697,16 @@ MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 
 	Assert(ap == nargs);		/* processed all actual parameters */
 
+	/* If we get here, the function did match all the supplied argnames */
+	*fgc_flags |= FGC_ARGNAMES_MATCH;
+
+	/* ... however, some of them might have been placed wrong */
+	if (arg_filled_twice)
+		return false;			/* some argname matched a positional argument */
+
+	/* If we get here, the call doesn't have invalid mixed notation */
+	*fgc_flags |= FGC_ARGNAMES_NONDUP;
+
 	/* Check for default arguments */
 	if (nargs < pronargs)
 	{
@@ -1682,6 +1725,9 @@ MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
 
 	Assert(ap == pronargs);		/* processed all function parameters */
 
+	/* If we get here, the call supplies all the required arguments */
+	*fgc_flags |= FGC_ARGNAMES_ALL;
+
 	return true;
 }
 
@@ -1745,11 +1791,13 @@ FunctionIsVisibleExt(Oid funcid, bool *is_missing)
 		char	   *proname = NameStr(procform->proname);
 		int			nargs = procform->pronargs;
 		FuncCandidateList clist;
+		int			fgc_flags;
 
 		visible = false;
 
 		clist = FuncnameGetCandidates(list_make1(makeString(proname)),
-									  nargs, NIL, false, false, false, false);
+									  nargs, NIL, false, false, false, false,
+									  &fgc_flags);
 
 		for (; clist; clist = clist->next)
 		{
@@ -1882,9 +1930,20 @@ OpernameGetOprid(List *names, Oid oprleft, Oid oprright)
  *
  * The returned items always have two args[] entries --- the first will be
  * InvalidOid for a prefix oprkind.  nargs is always 2, too.
+ *
+ * We return an empty list (NULL) if no suitable matches can be found.  If the
+ * operator name was schema-qualified with a schema that does not exist, then
+ * we return an empty list if missing_schema_ok is true and otherwise throw an
+ * error.  (missing_schema_ok does not affect the behavior otherwise.)
+ *
+ * The output argument *fgc_flags is filled with a bitmask indicating how
+ * far we were able to match the supplied information.  This is not of much
+ * interest if any candidates were found, but if not, it can help callers
+ * produce an on-point error message.
  */
 FuncCandidateList
-OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
+OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok,
+					  int *fgc_flags)
 {
 	FuncCandidateList resultList = NULL;
 	char	   *resultSpace = NULL;
@@ -1895,15 +1954,20 @@ OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
 	CatCList   *catlist;
 	int			i;
 
+	/* initialize output fgc_flags to empty */
+	*fgc_flags = 0;
+
 	/* deconstruct the name list */
 	DeconstructQualifiedName(names, &schemaname, &opername);
 
 	if (schemaname)
 	{
 		/* use exact schema given */
+		*fgc_flags |= FGC_SCHEMA_GIVEN; /* report that a schema is given */
 		namespaceId = LookupExplicitNamespace(schemaname, missing_schema_ok);
-		if (missing_schema_ok && !OidIsValid(namespaceId))
+		if (!OidIsValid(namespaceId))
 			return NULL;
+		*fgc_flags |= FGC_SCHEMA_EXISTS;	/* report that the schema exists */
 	}
 	else
 	{
@@ -1941,6 +2005,8 @@ OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
 		if (oprkind && operform->oprkind != oprkind)
 			continue;
 
+		*fgc_flags |= FGC_NAME_EXISTS;	/* the name is present in pg_operator */
+
 		if (OidIsValid(namespaceId))
 		{
 			/* Consider only opers in specified namespace */
@@ -2014,6 +2080,8 @@ OpernameGetCandidates(List *names, char oprkind, bool missing_schema_ok)
 			}
 		}
 
+		*fgc_flags |= FGC_NAME_VISIBLE; /* operator is in the right schema */
+
 		/*
 		 * Okay to add it to result list
 		 */