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

2 files changed, 188 insertions, 29 deletions

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 583bbbf232f..c43020a769d 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -42,6 +42,8 @@ typedef enum
 	FUNCLOOKUP_AMBIGUOUS,
 } FuncLookupError;
 
+static int	func_lookup_failure_details(int fgc_flags, List *argnames,
+										bool proc_call);
 static void unify_hypothetical_args(ParseState *pstate,
 									List *fargs, int numAggregatedArgs,
 									Oid *actual_arg_types, Oid *declared_arg_types);
@@ -115,6 +117,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 	int			nvargs;
 	Oid			vatype;
 	FuncDetailCode fdresult;
+	int			fgc_flags;
 	char		aggkind = 0;
 	ParseCallbackState pcbstate;
 
@@ -266,6 +269,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 	fdresult = func_get_detail(funcname, fargs, argnames, nargs,
 							   actual_arg_types,
 							   !func_variadic, true, proc_call,
+							   &fgc_flags,
 							   &funcid, &rettype, &retset,
 							   &nvargs, &vatype,
 							   &declared_arg_types, &argdefaults);
@@ -563,8 +567,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("procedure %s is not unique",
 							func_signature_string(funcname, nargs, argnames,
 												  actual_arg_types)),
-					 errhint("Could not choose a best candidate procedure. "
-							 "You might need to add explicit type casts."),
+					 errdetail("Could not choose a best candidate procedure."),
+					 errhint("You might need to add explicit type casts."),
 					 parser_errposition(pstate, location)));
 		else
 			ereport(ERROR,
@@ -572,8 +576,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("function %s is not unique",
 							func_signature_string(funcname, nargs, argnames,
 												  actual_arg_types)),
-					 errhint("Could not choose a best candidate function. "
-							 "You might need to add explicit type casts."),
+					 errdetail("Could not choose a best candidate function."),
+					 errhint("You might need to add explicit type casts."),
 					 parser_errposition(pstate, location)));
 	}
 	else
@@ -601,7 +605,9 @@ 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".
+		 * Depending on what was returned in fgc_flags, we can add some color
+		 * to that with detail or hint messages.
 		 */
 		if (list_length(agg_order) > 1 && !agg_within_group)
 		{
@@ -611,8 +617,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("function %s does not exist",
 							func_signature_string(funcname, nargs, argnames,
 												  actual_arg_types)),
-					 errhint("No aggregate function matches the given name and argument types. "
-							 "Perhaps you misplaced ORDER BY; ORDER BY must appear "
+					 errdetail("No aggregate function matches the given name and argument types."),
+					 errhint("Perhaps you misplaced ORDER BY; ORDER BY must appear "
 							 "after all regular arguments of the aggregate."),
 					 parser_errposition(pstate, location)));
 		}
@@ -622,8 +628,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("procedure %s does not exist",
 							func_signature_string(funcname, nargs, argnames,
 												  actual_arg_types)),
-					 errhint("No procedure matches the given name and argument types. "
-							 "You might need to add explicit type casts."),
+					 func_lookup_failure_details(fgc_flags, argnames,
+												 proc_call),
 					 parser_errposition(pstate, location)));
 		else
 			ereport(ERROR,
@@ -631,8 +637,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("function %s does not exist",
 							func_signature_string(funcname, nargs, argnames,
 												  actual_arg_types)),
-					 errhint("No function matches the given name and argument types. "
-							 "You might need to add explicit type casts."),
+					 func_lookup_failure_details(fgc_flags, argnames,
+												 proc_call),
 					 parser_errposition(pstate, location)));
 	}
 
@@ -905,6 +911,104 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 	return retval;
 }
 
+/*
+ * Interpret the fgc_flags and issue a suitable detail or hint message.
+ *
+ * Helper function to reduce code duplication while throwing a
+ * function-not-found error.
+ */
+static int
+func_lookup_failure_details(int fgc_flags, List *argnames, bool proc_call)
+{
+	/*
+	 * If not FGC_NAME_VISIBLE, we shouldn't raise the question of whether the
+	 * arguments are wrong.  If the function name was not schema-qualified,
+	 * it's helpful to distinguish between doesn't-exist-anywhere and
+	 * not-in-search-path; but if it was, there's really nothing to add to the
+	 * basic "function/procedure %s does not exist" message.
+	 *
+	 * Note: we passed missing_ok = false to FuncnameGetCandidates, so there's
+	 * no need to consider FGC_SCHEMA_EXISTS here: we'd have already thrown an
+	 * error if an explicitly-given schema doesn't exist.
+	 */
+	if (!(fgc_flags & FGC_NAME_VISIBLE))
+	{
+		if (fgc_flags & FGC_SCHEMA_GIVEN)
+			return 0;			/* schema-qualified name */
+		else if (!(fgc_flags & FGC_NAME_EXISTS))
+		{
+			if (proc_call)
+				return errdetail("There is no procedure of that name.");
+			else
+				return errdetail("There is no function of that name.");
+		}
+		else
+		{
+			if (proc_call)
+				return errdetail("A procedure of that name exists, but it is not in the search_path.");
+			else
+				return errdetail("A function of that name exists, but it is not in the search_path.");
+		}
+	}
+
+	/*
+	 * Next, complain if nothing had the right number of arguments.  (This
+	 * takes precedence over wrong-argnames cases because we won't even look
+	 * at the argnames unless there's a workable number of arguments.)
+	 */
+	if (!(fgc_flags & FGC_ARGCOUNT_MATCH))
+	{
+		if (proc_call)
+			return errdetail("No procedure of that name accepts the given number of arguments.");
+		else
+			return errdetail("No function of that name accepts the given number of arguments.");
+	}
+
+	/*
+	 * If there are argnames, and we failed to match them, again we should
+	 * mention that and not bring up the argument types.
+	 */
+	if (argnames != NIL && !(fgc_flags & FGC_ARGNAMES_MATCH))
+	{
+		if (proc_call)
+			return errdetail("No procedure of that name accepts the given argument names.");
+		else
+			return errdetail("No function of that name accepts the given argument names.");
+	}
+
+	/*
+	 * We could have matched all the given argnames and still not have had a
+	 * valid call, either because of improper use of mixed notation, or
+	 * because of missing arguments, or because the user misused VARIADIC. The
+	 * rules about named-argument matching are finicky enough that it's worth
+	 * trying to be specific about the problem.  (The messages here are chosen
+	 * with full knowledge of the steps that namespace.c uses while checking a
+	 * potential match.)
+	 */
+	if (argnames != NIL && !(fgc_flags & FGC_ARGNAMES_NONDUP))
+		return errdetail("In the closest available match, "
+						 "an argument was specified both positionally and by name.");
+
+	if (argnames != NIL && !(fgc_flags & FGC_ARGNAMES_ALL))
+		return errdetail("In the closest available match, "
+						 "not all required arguments were supplied.");
+
+	if (argnames != NIL && !(fgc_flags & FGC_ARGNAMES_VALID))
+		return errhint("This call would be correct if the variadic array were labeled VARIADIC and placed last.");
+
+	if (fgc_flags & FGC_VARIADIC_FAIL)
+		return errhint("The VARIADIC parameter must be placed last, even when using argument names.");
+
+	/*
+	 * Otherwise, the problem must be incorrect argument types.
+	 */
+	if (proc_call)
+		(void) errdetail("No procedure of that name accepts the given argument types.");
+	else
+		(void) errdetail("No function of that name accepts the given argument types.");
+	return errhint("You might need to add explicit type casts.");
+}
+
 
 /* func_match_argtypes()
  *
@@ -1372,9 +1476,14 @@ func_select_candidate(int nargs,
  *	1) check for possible interpretation as a type coercion request
  *	2) apply the ambiguous-function resolution rules
  *
- * Return values *funcid through *true_typeids receive info about the function.
- * If argdefaults isn't NULL, *argdefaults receives a list of any default
- * argument expressions that need to be added to the given arguments.
+ * If there is no match at all, we return FUNCDETAIL_NOTFOUND, and *fgc_flags
+ * is filled with some flags that may be useful for issuing an on-point error
+ * message (see FuncnameGetCandidates).
+ *
+ * On success, return values *funcid through *true_typeids receive info about
+ * the function.  If argdefaults isn't NULL, *argdefaults receives a list of
+ * any default argument expressions that need to be added to the given
+ * arguments.
  *
  * When processing a named- or mixed-notation call (ie, fargnames isn't NIL),
  * the returned true_typeids and argdefaults are ordered according to the
@@ -1400,6 +1509,7 @@ func_get_detail(List *funcname,
 				bool expand_variadic,
 				bool expand_defaults,
 				bool include_out_arguments,
+				int *fgc_flags, /* return value */
 				Oid *funcid,	/* return value */
 				Oid *rettype,	/* return value */
 				bool *retset,	/* return value */
@@ -1424,7 +1534,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,
+										   fgc_flags);
 
 	/*
 	 * Quickly check if there is an exact match to the input datatypes (there
@@ -1594,7 +1705,10 @@ func_get_detail(List *funcname,
 		 */
 		if (fargnames != NIL && !expand_variadic && nargs > 0 &&
 			best_candidate->argnumbers[nargs - 1] != nargs - 1)
+		{
+			*fgc_flags |= FGC_VARIADIC_FAIL;
 			return FUNCDETAIL_NOTFOUND;
+		}
 
 		*funcid = best_candidate->oid;
 		*nvargs = best_candidate->nvargs;
@@ -2053,6 +2167,7 @@ LookupFuncNameInternal(ObjectType objtype, List *funcname,
 {
 	Oid			result = InvalidOid;
 	FuncCandidateList clist;
+	int			fgc_flags;
 
 	/* NULL argtypes allowed for nullary functions only */
 	Assert(argtypes != NULL || nargs == 0);
@@ -2062,7 +2177,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,
+								  &fgc_flags);
 
 	/* 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/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index 0c4337563cf..7bd7a336fd6 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -72,7 +72,8 @@ static FuncDetailCode oper_select_candidate(int nargs,
 											Oid *operOid);
 static void op_error(ParseState *pstate, List *op,
 					 Oid arg1, Oid arg2,
-					 FuncDetailCode fdresult, int location);
+					 FuncDetailCode fdresult, int fgc_flags, int location);
+static int	oper_lookup_failure_details(int fgc_flags, bool is_unary_op);
 static bool make_oper_cache_key(ParseState *pstate, OprCacheKey *key,
 								List *opname, Oid ltypeId, Oid rtypeId,
 								int location);
@@ -373,6 +374,7 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId,
 	Oid			operOid;
 	OprCacheKey key;
 	bool		key_ok;
+	int			fgc_flags = 0;
 	FuncDetailCode fdresult = FUNCDETAIL_NOTFOUND;
 	HeapTuple	tup = NULL;
 
@@ -404,7 +406,7 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId,
 		FuncCandidateList clist;
 
 		/* Get binary operators of given name */
-		clist = OpernameGetCandidates(opname, 'b', false);
+		clist = OpernameGetCandidates(opname, 'b', false, &fgc_flags);
 
 		/* No operators found? Then fail... */
 		if (clist != NULL)
@@ -434,7 +436,8 @@ oper(ParseState *pstate, List *opname, Oid ltypeId, Oid rtypeId,
 			make_oper_cache_entry(&key, operOid);
 	}
 	else if (!noError)
-		op_error(pstate, opname, ltypeId, rtypeId, fdresult, location);
+		op_error(pstate, opname, ltypeId, rtypeId,
+				 fdresult, fgc_flags, location);
 
 	return (Operator) tup;
 }
@@ -520,6 +523,7 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
 	Oid			operOid;
 	OprCacheKey key;
 	bool		key_ok;
+	int			fgc_flags = 0;
 	FuncDetailCode fdresult = FUNCDETAIL_NOTFOUND;
 	HeapTuple	tup = NULL;
 
@@ -551,7 +555,7 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
 		FuncCandidateList clist;
 
 		/* Get prefix operators of given name */
-		clist = OpernameGetCandidates(op, 'l', false);
+		clist = OpernameGetCandidates(op, 'l', false, &fgc_flags);
 
 		/* No operators found? Then fail... */
 		if (clist != NULL)
@@ -585,7 +589,8 @@ left_oper(ParseState *pstate, List *op, Oid arg, bool noError, int location)
 			make_oper_cache_entry(&key, operOid);
 	}
 	else if (!noError)
-		op_error(pstate, op, InvalidOid, arg, fdresult, location);
+		op_error(pstate, op, InvalidOid, arg,
+				 fdresult, fgc_flags, location);
 
 	return (Operator) tup;
 }
@@ -621,30 +626,68 @@ op_signature_string(List *op, Oid arg1, Oid arg2)
 static void
 op_error(ParseState *pstate, List *op,
 		 Oid arg1, Oid arg2,
-		 FuncDetailCode fdresult, int location)
+		 FuncDetailCode fdresult, int fgc_flags, int location)
 {
 	if (fdresult == FUNCDETAIL_MULTIPLE)
 		ereport(ERROR,
 				(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
 				 errmsg("operator is not unique: %s",
 						op_signature_string(op, arg1, arg2)),
-				 errhint("Could not choose a best candidate operator. "
-						 "You might need to add explicit type casts."),
+				 errdetail("Could not choose a best candidate operator."),
+				 errhint("You might need to add explicit type casts."),
 				 parser_errposition(pstate, location)));
 	else
 		ereport(ERROR,
 				(errcode(ERRCODE_UNDEFINED_FUNCTION),
 				 errmsg("operator does not exist: %s",
 						op_signature_string(op, arg1, arg2)),
-				 (!arg1 || !arg2) ?
-				 errhint("No operator matches the given name and argument type. "
-						 "You might need to add an explicit type cast.") :
-				 errhint("No operator matches the given name and argument types. "
-						 "You might need to add explicit type casts."),
+				 oper_lookup_failure_details(fgc_flags, (!arg1 || !arg2)),
 				 parser_errposition(pstate, location)));
 }
 
 /*
+ * Interpret the fgc_flags and issue a suitable detail or hint message.
+ */
+static int
+oper_lookup_failure_details(int fgc_flags, bool is_unary_op)
+{
+	/*
+	 * If not FGC_NAME_VISIBLE, we shouldn't raise the question of whether the
+	 * arguments are wrong.  If the operator name was not schema-qualified,
+	 * it's helpful to distinguish between doesn't-exist-anywhere and
+	 * not-in-search-path; but if it was, there's really nothing to add to the
+	 * basic "operator does not exist" message.
+	 *
+	 * Note: we passed missing_ok = false to OpernameGetCandidates, so there's
+	 * no need to consider FGC_SCHEMA_EXISTS here: we'd have already thrown an
+	 * error if an explicitly-given schema doesn't exist.
+	 */
+	if (!(fgc_flags & FGC_NAME_VISIBLE))
+	{
+		if (fgc_flags & FGC_SCHEMA_GIVEN)
+			return 0;			/* schema-qualified name */
+		else if (!(fgc_flags & FGC_NAME_EXISTS))
+			return errdetail("There is no operator of that name.");
+		else
+			return errdetail("An operator of that name exists, but it is not in the search_path.");
+	}
+
+	/*
+	 * Otherwise, the problem must be incorrect argument type(s).
+	 */
+	if (is_unary_op)
+	{
+		(void) errdetail("No operator of that name accepts the given argument type.");
+		return errhint("You might need to add an explicit type cast.");
+	}
+	else
+	{
+		(void) errdetail("No operator of that name accepts the given argument types.");
+		return errhint("You might need to add explicit type casts.");
+	}
+}
+
+/*
  * make_op()
  *		Operator expression construction.
  *