Re: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name

David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> On Thu, 21 Feb 2019 at 09:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The real problem here is that you've unilaterally decided that all callers
>> of get_object_address() need a particular behavior --- and not only that,
>> but a behavior that seems fairly surprising and unprincipled, given that
>> get_object_address's option is documented as "missing_ok" (something the
>> patch doesn't even bother to change).
>> ...
>> Should we be propagating that 3-way flag further up, to
>> get_object_address() callers? I dunno.

> I don't see why that's needed given what's mentioned above.

Well, if we're not going to propagate the option further up, then I think
we might as well do it like you originally suggested, ie leave the
signature of LookupFuncName alone and just document that the
ambiguous-function case is not covered by noError. As far as I can tell,
just about all the other callers besides get_object_address() have no
interest in this issue because they're not passing nargs == -1.
What's more, a lot of them look like this example in
findRangeSubtypeDiffFunction:

procOid = LookupFuncName(procname, 2, argList, FUNCLOOKUP_NOERROR);

if (!OidIsValid(procOid))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_FUNCTION),
errmsg("function %s does not exist",
func_signature_string(procname, 2, NIL, argList))));

so that if some day in the future FUNCLOOKUP_NOERROR could actually
suppress an ambiguous-function error here, the caller would proceed
to report an incorrect/misleading error message. It doesn't seem to
make much sense to allow callers to separately suppress or not
suppress ambiguous-function unless we also change the return
convention so that the callers can tell which case happened.
And that's looking a bit pointless, at least for now.

So, sorry for making you chase down this dead end, but it wasn't
obvious until now (to me anyway) that it was a dead end.

I did notice though that the patch fails to cover the same problem
right next door for procedures:

regression=# create procedure funcp(param1 text) language sql as 'select $1';
CREATE PROCEDURE
regression=# create procedure funcp(param1 int) language sql as 'select $1';
CREATE PROCEDURE
regression=# drop procedure funcp;
ERROR: could not find a procedure named "funcp"

This should surely be complaining about ambiguity, rather than giving
the same error text as if there were zero matches.

Possibly the same occurs for aggregates, though I'm not sure if that
code is reachable --- DROP AGGREGATE, at least, won't let you omit the
arguments.

I think the underlying cause of this is that LookupFuncWithArgs is in
the same situation I just complained for outside callers: it cannot tell
whether its noError request suppressed a not-found or ambiguous-function
case. Maybe the way to proceed here is to refactor within parse_func.c
so that there's an underlying function that returns an indicator of what
happened, and both LookupFuncName and LookupFuncWithArgs call it, each
with their own ideas about how to phrase the error reports. It's
certainly mighty ugly that LookupFuncWithArgs farms out the actual
error report in some cases and not others.

regards, tom lane