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

On Mon, 4 Mar 2019 at 09:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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:
> >> 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.

Okay.

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

I started working on this, but... damage control... I don't want to
take it too far without you having a glance at it first.

I've invented a new function by the name of LookupFuncNameInternal().
This attempts to find the function, but if it can't or the name is
ambiguous then it returns InvalidOid and sets an error code parameter.
I've made both LookupFuncName and LookupFuncWithArgs use this.

In my travels, I discovered something else that does not seem very great.

postgres=# create procedure abc(int) as $$ begin end; $$ language plpgsql;
CREATE PROCEDURE
postgres=# drop function if exists abc(int);
NOTICE: function abc(pg_catalog.int4) does not exist, skipping
DROP FUNCTION

I think it would be better to ERROR in that case. So with the attached
we now get:

postgres=# create procedure abc(int) as $$ begin end; $$ language plpgsql;
CREATE PROCEDURE
postgres=# drop function if exists abc(int);
ERROR: abc(integer) is not a function

I've also tried to have the error messages mention procedure when the
object is a procedure and function when its a function. It looks like
the previous code was calling LookupFuncName() with noError=true so it
could handle using "procedure" in the error messages itself, but it
failed to do that for an ambiguous procedure name. That should now be
fixed.

I also touched the too many function arguments case, but perhaps I
need to go further there and do something for aggregates. I've not
thought too hard about that.

I've not really read the patch back or done any polishing yet. Manual
testing done is minimal, and I didn't add tests for the new behaviour
change either. I can do more if the feedback is positive.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services