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

On Thu, 21 Feb 2019 at 09:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > On Wed, 20 Feb 2019 at 18:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >> I think that the right call is to add the object type into the
> >> arguments of LookupFuncName().
>
> I'm not clear how that helps exactly?
>
> > But there are plenty of callers that use LookupFuncName() directly. Do
> > you happen to know it's okay for all those to error out with the
> > additional error conditions that such a change would move into that
> > function? I certainly don't know that.
>
> 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). It's not very apparent to me why
> function-related lookups should start behaving differently from other
> lookups in that function, and it's sure not apparent that all callers of
> get_object_address() are on board with it.

I assume you're talking about:

* If the object is not found, an error is thrown, unless missing_ok is
* true. In this case, no lock is acquired, relp is set to NULL, and the
* returned address has objectId set to InvalidOid.

Well, I didn't update that comment because the code I've changed does
nothing different for the missing_ok case. The missing function error
is still raised or not raised correctly depending on the value of that
flag.

I understand your original gripe with the patch where I had changed
the meaning of noError to mean
"noError-Apart-From-If-Its-An-Ambiguous-Function", without much of any
documentation to mention that fact, but it seems to me that this time
around your confusing missing_ok with noError. To me noError means
don't raise an error, and missing_ok is intended for use with IF [NOT]
EXISTS... Yes, it might be getting used for something else, but since
we still raise an error when the function is missing when the flag is
set to false and don't when it's set to true. I fail to see why that
breaks the contract that's documented in the above comment. If you
think it does then please explain why.

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

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