Identifying function-lookup failures due to argument name mismatches

I wrote:

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.

I couldn't quite let go of this, and after some thought I hit on the
idea of making FuncnameGetCandidates pass back a bitmask of flags
showing how far the match succeeded. This seems to work pretty
nicely, allowing quite-detailed reports with only minimal added
overhead or code restructuring. There's probably room for further
improvement, but it has less of a whiff of "quick single-purpose
hack". See draft commit message for more details.

regards, tom lane

On Thu, Aug 14, 2025 at 9:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

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.

I couldn't quite let go of this, and after some thought I hit on the
idea of making FuncnameGetCandidates pass back a bitmask of flags
showing how far the match succeeded. This seems to work pretty
nicely, allowing quite-detailed reports with only minimal added
overhead or code restructuring. There's probably room for further
improvement, but it has less of a whiff of "quick single-purpose
hack". See draft commit message for more details.

As the original "complainer", I felt compelled to have a look at this
patch. Not to gauge its quality, mind you; I'm not qualified for
that. But to look at the flags, and the error messages. Below are my
notes observations:

From the use in code:
FGC_SCHEMA_MATCH; /* report that the schema is valid */
FGC_NAME_MATCH; /* we found a matching function name */
FGC_ARGNAMES_VALID; /* We found a fully-valid call using argument names */
FGC_ARGNAMES_MATCH; /* the function did match all the supplied argnames */
FGC_ARGNAMES_PLACED; /* call doesn't violate the rules for mixed notation */
FGC_ARGNAMES_ALL; /* call supplies all the required arguments */
FGC_VARIADIC_FAIL;

From the declaration:
+/*
+ * FuncnameGetCandidates also returns a bitmask containing these flags,
+ * which report on what it found or didn't find.  They can help callers
+ * produce better error reports after a function lookup failure.
+ */
+#define FGC_SCHEMA_MATCH 0x0001 /* Found the explicitly-specified schema */
+#define FGC_NAME_MATCH 0x0002 /* Found a function name match */
+/* These bits relate only to calls using named or mixed arguments: */
+#define FGC_ARGNAMES_MATCH 0x0004 /* Found a func matching all argnames */
+#define FGC_ARGNAMES_PLACED 0x0008 /* Found argnames validly placed */
+#define FGC_ARGNAMES_ALL 0x0010 /* Found a function with no missing args */
+#define FGC_ARGNAMES_VALID 0x0020 /* Found a fully-valid use of argnames */
+/* These bits are actually filled by func_get_detail: */
+#define FGC_VARIADIC_FAIL 0x0040 /* Disallowed VARIADIC with named args */

I like the new messages in func_lookup_failure_details(). Very much
so in fact. BUT I still don't like the fallback "traditional"
message, because the way I read it, it fails to mention argument
*names* could be the reason for the lookup failure. Now maybe that's
moot, because of earlier messages. But I can't know that myself, thus
I'm still re-iterating my feeling on this. In my reading, "given
name" applies to "function" or "procedure" before it, and not to
"argument" after it. Thus I'd go with "and argument types or names".

One thing I find missing are flags about the actual syntax used by the
user, i.e.
is it schema-qualified?
does it use named arguments?

If some flags you've added Tom, are TRUE, then that's implied.
But is the converse true? i.e. if the flag is FALSE, can you know
whether named-args were used? (schema-qualified seems special, as
fails earlier, if I read you right). Because it could have some
bearing on the errors, no?

In any case, what you are proposing goes a LONG WAY to improve the
current situation, IMHO. Thank you VERY MUCH for pursuing this. And
I very much hope something like it goes through for v19 next year.
--DD

Hi Tom,

On Aug 8, 2025, at 09:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Looks like you are looking for someone to work out a final patch. If that is true, I will be happy to work on this problem.

I couldn't quite let go of this, and after some thought I hit on the
idea of making FuncnameGetCandidates pass back a bitmask of flags
showing how far the match succeeded. This seems to work pretty
nicely, allowing quite-detailed reports with only minimal added
overhead or code restructuring. There's probably room for further
improvement, but it has less of a whiff of "quick single-purpose
hack". See draft commit message for more details.

I traced this problem today, and I agree that making FuncnameGetCandidates to pass out some information should be right direction to go.

When there are multiple matches, I think we can find the best match by considering argument names/types, default values. If there are still multiple best matches, I think we can prompt all matches to client.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Dominique Devienne <ddevienne@gmail.com> writes:

I like the new messages in func_lookup_failure_details(). Very much
so in fact. BUT I still don't like the fallback "traditional"
message, because the way I read it, it fails to mention argument
*names* could be the reason for the lookup failure. Now maybe that's
moot, because of earlier messages. But I can't know that myself, thus
I'm still re-iterating my feeling on this. In my reading, "given
name" applies to "function" or "procedure" before it, and not to
"argument" after it. Thus I'd go with "and argument types or names".

Dunno, I think the new messages already cover all the interesting
cases of argument name mismatch. I'm hesitant to touch the
longstanding hint, and if I did I'd probably change it more than that,
to something like

ERROR: function foo(integer) does not exist
DETAIL: No function of that name matches the given argument types.
HINT: You might need to add explicit type casts.

because whoever wrote it originally had a poor grasp of our
error message style guide. But that'd result in very widespread
changes in our regression test outputs, probably likewise break
the regression tests of extensions and other downstream code,
and generally cause a lot more pain than I think it's worth.
(Maybe others think differently?)

Perhaps a compromise could be to use two different hint messages,
mentioning argument names only when the call actually used some.
That would limit the blast radius a good deal, I think, though
I didn't try counting affected tests.

One thing I find missing are flags about the actual syntax used by the
user, i.e.
is it schema-qualified?
does it use named arguments?

I thought that wasn't really necessary, because the caller already
knows those things, or can discover them about as easily as by
checking a flag.

If some flags you've added Tom, are TRUE, then that's implied.
But is the converse true? i.e. if the flag is FALSE, can you know
whether named-args were used? (schema-qualified seems special, as
fails earlier, if I read you right). Because it could have some
bearing on the errors, no?

The patch's func_lookup_failure_details() code shows how I intended
to deal with those questions. Sure, we could also do that through
returned flags, but is that better? ParseFuncOrColumn contains
existing tests for "argnames != NIL", so checking that the same way
in this new code seemed better than doing it differently.

regards, tom lane

Chao Li <li.evan.chao@gmail.com> writes:

On Aug 8, 2025, at 09:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I couldn't quite let go of this, and after some thought I hit on the
idea of making FuncnameGetCandidates pass back a bitmask of flags
showing how far the match succeeded.

I traced this problem today, and I agree that making FuncnameGetCandidates to pass out some information should be right direction to go.

When there are multiple matches, I think we can find the best match by considering argument names/types, default values. If there are still multiple best matches, I think we can prompt all matches to client.

I don't want to touch the existing rules about how we winnow down the
potential matches. That has a risk of breaking applications that are
fine today. The idea of this patch is just to give more-specific
error messages when we end up with no matches. (In fact, one of the
points that I think could use review is checking that the small
refactoring I did have to do inside MatchNamedCall didn't change
its existing outputs.)

regards, tom lane

I wrote:

Dunno, I think the new messages already cover all the interesting
cases of argument name mismatch. I'm hesitant to touch the
longstanding hint, and if I did I'd probably change it more than that,
to something like

ERROR: function foo(integer) does not exist
DETAIL: No function of that name matches the given argument types.
HINT: You might need to add explicit type casts.

because whoever wrote it originally had a poor grasp of our
error message style guide. But that'd result in very widespread
changes in our regression test outputs, probably likewise break
the regression tests of extensions and other downstream code,
and generally cause a lot more pain than I think it's worth.
(Maybe others think differently?)

I decided to investigate just how bad changing this would be, and it
seems maybe not *that* awful. v3-0001 attached is much like v2-0001,
and then 0002 shows the effects of changing the wording of this hint,
and 0003 and 0004 explore cleaning up some related messages. I count
the following numbers of changed messages in each patch:

$ grep '^-HINT' v3-0001-Provide-more-specific-error-hints-for-function-lo.patch | wc
17 306 1803
$ grep '^-HINT' v3-0002-Change-the-wording-of-our-traditional-function-no.patch | wc
40 715 4214
$ grep '^-HINT' v3-0003-Improve-the-messages-for-operator-not-found-too.patch | wc
19 342 2014
$ grep '^-HINT' v3-0004-Mop-up-a-few-other-error-message-style-violations.patch | wc
7 109 644

So doing all of this is certainly a little bit invasive, but it's not
out of the question IMO. On the other hand it could certainly be
argued that 0002-0004 are just style nannyism.

0001 makes a couple of changes compared to v2. I adopted your thought
of passing back a flag bit about a schema name being given after all.
I concluded that was a bit cleaner than the other way. I still think
it's best for ParseFuncOrColumn to uniformly use "argnames != NIL"
for checking whether there are argnames, though. Also, I added a
flag bit and error message for the case where none of the candidate
functions have the right number of arguments, because when that's
true, we'll never get to looking at argument names or types. And
I switched some of the messages from HINT to DETAIL.

regards, tom lane

On Fri, Aug 22, 2025 at 12:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

Dunno, I think the new messages already cover all the interesting
cases of argument name mismatch. I'm hesitant to touch the
longstanding hint, and if I did I'd probably change it more than that,
to something like

ERROR: function foo(integer) does not exist
DETAIL: No function of that name matches the given argument types.
HINT: You might need to add explicit type casts.

because whoever wrote it originally had a poor grasp of our
error message style guide. But that'd result in very widespread
changes in our regression test outputs, probably likewise break
the regression tests of extensions and other downstream code,
and generally cause a lot more pain than I think it's worth.
(Maybe others think differently?)

I decided to investigate [...] it seems maybe not *that* awful.

Great.

0001 makes a couple of changes compared to v2. I adopted your thought
of passing back a flag bit about a schema name being given after all.
I concluded that was a bit cleaner than the other way.

Excellent. Thanks for sharing.
Maybe I'll get another undeserved medallion then :)

it's best for ParseFuncOrColumn to uniformly use "argnames != NIL"
for checking whether there are argnames, though.

I'm sure you're right. But given the above, an out flag for it too
would be more consistent, like the schema one. My $0.02.

Also, I added a flag bit [...] where none of the candidate [...]
because when that's true, we'll never get to looking at arguments

Sounds like an improvement indeed. Subtle difference I didn't
even get on my first reading of your mail. You're into it now!

One last though. Is it worth reserving a few bits to count the
candidate matches? You'll never reach 32 flags, so 8 feels like plenty.
Barring listing the candidates, a count hint might help? In my case
it was only 1, but it more complete cases where the search_path
is involved, one might get surprised with candidates coming from afar
making things ambiguous? Again, jus thinking aloud. --DD

Dominique Devienne <ddevienne@gmail.com> writes:

One last though. Is it worth reserving a few bits to count the
candidate matches? You'll never reach 32 flags, so 8 feels like plenty.
Barring listing the candidates, a count hint might help? In my case
it was only 1, but it more complete cases where the search_path
is involved, one might get surprised with candidates coming from afar
making things ambiguous? Again, jus thinking aloud. --DD

Candidates in what sense, that is where would you make the count?
In any case, that seems like it's about adding detail to the
"ambiguous function" case, which might be worth doing but it's
not the goal of this patch.

regards, tom lane

Here is a v4 with some additional bike-shedding on the error texts.
In particular, I decided that it was worth expending an additional
flag bit so that we could reliably distinguish "There is no function
of that name" from "A function of that name exists, but it is not in
the search_path". (Since FuncnameGetCandidates is already searching
the entire set of functions matching the given name, it doesn't take
any extra work to know that there's a match outside the search path.)
I rephrased a couple of the other messages too, but without any
substantive logic change.

regards, tom lane

On Aug 25, 2025, at 02:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Here is a v4 with some additional bike-shedding on the error texts.
In particular, I decided that it was worth expending an additional
flag bit so that we could reliably distinguish "There is no function
of that name" from "A function of that name exists, but it is not in
the search_path". (Since FuncnameGetCandidates is already searching
the entire set of functions matching the given name, it doesn't take
any extra work to know that there's a match outside the search path.)
I rephrased a couple of the other messages too, but without any
substantive logic change.

I tested various error cases, all got proper error messages. So, I think this patch has significantly improved the situation.

I just have a tiny comment. In func_lookup_failure_details(), there are a lot of duplicate code like:

```
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.”);
```

The if-else is just to distinguish “procedure” and “function”, rest of words are duplicated.

Can we avoid the duplication in a way like:

```
static int
func_lookup_failure_details(int fgc_flags, List *argnames, bool proc_call)
{
const char *func_kind = proc_call ? "procedure" : "function";

/*
if (proc_call)
return errdetail("There is no procedure of that name.");
else
return errdetail("There is no function of that name.");
*/
return errdetail("There is no %s of that name.", func_kind);
```

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 24.08.25 20:47, Tom Lane wrote:

Here is a v4 with some additional bike-shedding on the error texts.
In particular, I decided that it was worth expending an additional
flag bit so that we could reliably distinguish "There is no function
of that name" from "A function of that name exists, but it is not in
the search_path". (Since FuncnameGetCandidates is already searching
the entire set of functions matching the given name, it doesn't take
any extra work to know that there's a match outside the search path.)
I rephrased a couple of the other messages too, but without any
substantive logic change.

I only gave it a quick review right now. I have also been wanting to
make the function lookup error messages more specific, so I like this
direction very much. The wording of the messages looks good and more
useful than before.

On 25.08.25 04:43, Chao Li wrote:

Can we avoid the duplication in a way like:

```
static int
func_lookup_failure_details(int fgc_flags, List *argnames, bool proc_call)
{
const char *func_kind = proc_call ? "procedure" : "function";

/*
if (proc_call)
return errdetail("There is no procedure of that name.");
else
return errdetail("There is no function of that name.");
*/
return errdetail("There is no %s of that name.", func_kind);
```

No, see here:
https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES

On Aug 27, 2025, at 23:42, Peter Eisentraut <peter@eisentraut.org> wrote:

On 25.08.25 04:43, Chao Li wrote:

Can we avoid the duplication in a way like:
```
static int
func_lookup_failure_details(int fgc_flags, List *argnames, bool proc_call)
{
const char *func_kind = proc_call ? "procedure" : "function";
/*
if (proc_call)
return errdetail("There is no procedure of that name.");
else
return errdetail("There is no function of that name.");
*/
return errdetail("There is no %s of that name.", func_kind);
```

No, see here: https://www.postgresql.org/docs/devel/nls-programmer.html#NLS-GUIDELINES

Thank you Peter very much. It is good to learn.

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Some review of 0001:

+ return errdetail("A procedure of that name exists, but it is not in
the search_path.");

Are you sure you want to expose this case in this way? From a security
point of view it makes me a bit nervous. If we're going to keep it, it
should have a test. Even from a non-security perspective, maybe having
the error message vary based on the contents of a completely unrelated
schema is not the best design decision. I can imagine that hosing some
user that is looking for a specific message and then somebody installs
an extension and the message changes even though there's no reason for
them to interact.

-HINT: No function matches the given name and argument types. You
might need to add explicit type casts.

I wonder what caused this line to disappear without being replaced by
anything (test_extensions.out).

Overall, I like this. I think these changes are helpful.

Some review of 0002:

I don't mind the churn. It is perhaps not mandatory, though. Call me +0.5.

Comments above also basically apply to 0003 and 0004: not mandatory, I
tentatively think they're improvements, be careful about the
not-in-schema case, test it if we're going to expose that information.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

Some review of 0001:
+ return errdetail("A procedure of that name exists, but it is not in
the search_path.");

Are you sure you want to expose this case in this way? From a security
point of view it makes me a bit nervous.

I'm not seeing the security concern? An attacker who can issue a
SQL command that would trigger this could presumably also issue a
"SELECT FROM pg_proc" that would reveal the same info and more.

If we're going to keep it, it
should have a test.

Huh, I thought I had covered the case, but you're right it's not
anywhere in the .out files. Will fix.

Even from a non-security perspective, maybe having
the error message vary based on the contents of a completely unrelated
schema is not the best design decision. I can imagine that hosing some
user that is looking for a specific message and then somebody installs
an extension and the message changes even though there's no reason for
them to interact.

The primary error message is not varying, only the DETAIL/HINT, so
I find this concern pretty far-fetched. Also, I believe that the
case that the message intends to help with is very common and so
it will save a lot of people time, more than enough to outweigh
any cases where it's perhaps un-optimal.

-HINT: No function matches the given name and argument types. You
might need to add explicit type casts.

I wonder what caused this line to disappear without being replaced by
anything (test_extensions.out).

That is the response to

ERROR: function public.dep_req2() does not exist
LINE 1: SELECT public.dep_req2() || ' req3b'
^
-HINT: No function matches the given name and argument types. You might need to add explicit type casts.

and I omitted the hint/detail because it seems to add nothing,
per this argument:

+     * 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.

I'm certainly willing to discuss that choice, but I wonder what you
have in mind that isn't just a restatement of "function does not
exist". There flat out isn't a pg_proc entry of the name that
the user gave. We could issue something like "HINT: maybe you
misspelled the function name." or "HINT: maybe there's some extension
you need to install." but that's getting way too nanny-ish for my
taste. The odds of giving an on-point hint don't seem good.

Overall, I like this. I think these changes are helpful.
Some review of 0002:
I don't mind the churn. It is perhaps not mandatory, though. Call me +0.5.
Comments above also basically apply to 0003 and 0004: not mandatory, I
tentatively think they're improvements, be careful about the
not-in-schema case, test it if we're going to expose that information.

Fair enough.

regards, tom lane

On Mon, Sep 15, 2025 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary error message is not varying, only the DETAIL/HINT, so
I find this concern pretty far-fetched. Also, I believe that the
case that the message intends to help with is very common and so
it will save a lot of people time, more than enough to outweigh
any cases where it's perhaps un-optimal.

I'm not entirely convinced, but you could well be right. I do like all
the other detailed diagnostics, I think, I just wasn't sure about that
one. But I'm not really here to argue, just giving my opinion.

That is the response to

ERROR: function public.dep_req2() does not exist
LINE 1: SELECT public.dep_req2() || ' req3b'
^
-HINT: No function matches the given name and argument types. You might need to add explicit type casts.

and I omitted the hint/detail because it seems to add nothing,

Yeah, OK, that's fair.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Sep 15, 2025 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary error message is not varying, only the DETAIL/HINT, so
I find this concern pretty far-fetched. Also, I believe that the
case that the message intends to help with is very common and so
it will save a lot of people time, more than enough to outweigh
any cases where it's perhaps un-optimal.

I'm not entirely convinced, but you could well be right. I do like all
the other detailed diagnostics, I think, I just wasn't sure about that
one. But I'm not really here to argue, just giving my opinion.

Fair enough. Again, how shall we proceed? What I suggest is to
go ahead and push what I have, and if there's anything that's not
so great, hopefully we'll get feedback about it before v19 is
frozen.

regards, tom lane

On Mon, Sep 15, 2025 at 5:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Sep 15, 2025 at 4:01 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

The primary error message is not varying, only the DETAIL/HINT, so
I find this concern pretty far-fetched. Also, I believe that the
case that the message intends to help with is very common and so
it will save a lot of people time, more than enough to outweigh
any cases where it's perhaps un-optimal.

I'm not entirely convinced, but you could well be right. I do like all
the other detailed diagnostics, I think, I just wasn't sure about that
one. But I'm not really here to argue, just giving my opinion.

Fair enough. Again, how shall we proceed? What I suggest is to
go ahead and push what I have, and if there's anything that's not
so great, hopefully we'll get feedback about it before v19 is
frozen.

Seems reasonable. I don't see that anyone is strongly objecting. In
fact, I think everyone who has commented has been generally in favor,
just with various minor concerns here and there. And it's certainly
better to put stuff that might need some fine-tuning into the tree
sooner rather than later.

--
Robert Haas
EDB: http://www.enterprisedb.com