Re: Cache lookup errors with functions manipulation object addresses

On Thu, Jul 20, 2017 at 6:26 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier wrote:
>> On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>> > I think the addition of checks everywhere for NULL return is worse.
>> > Let's add a missing_ok flag instead, so that most callers can just trust
>> > that they get a non null value if they don't want to deal with that
>> > case.
>>
>> If you want to minimize the diffs or such a patch, we could as well
>> have an extended version of those APIs. I don't think that for the
>> addition of one argument like a missing_ok that's the way to go, but
>> some people may like that to make this patch less intrusive.
>
> I think minimizing API churn is worthwhile in some cases but not all.
> These functions seem fringe enough that not having an API-compatible
> version is unnecessary. But that's just my opinion.

I can see your point. The v1 proposed above adds quite a lot of error
code churn to deal with the lack of missing_ok flag in
getObjectDescription, getObjectIdentity and getObjectIdentityParts.
And the cache lookup error messages cannot be object-specific either,
so I fell back to using %u/%u with the class as first identifier.
Let's go with what you suggest here then.

Before producing any v2, I would still need some sort of consensus
about a couple of points though to grab object details. Here is what I
think should be done:
1) For functions, let's remove format_procedure_qualified, and replace
it with format_procedure_extended which replaces
format_procedure_internal.
2) For relation attributes, we have now get_relid_attribute_name()
which cannot tolerate failures, and get_attname which returns NULL on
errors. My suggestion here is to remove get_relid_attribute_name, and
add a missing_ok flag to get_attname. Let's do this as a refactoring
patch. One thing that may matter is modules calling the existing APIs.
We could provide a compatibility macro.
3) For types, the existing interface is more a mess. HEAD has
allow_invalid which is used by the SQL function format_type. My
suggestion here would be to remove allow_invalid and replace it with
missing_ok, to return NULL if the type has gone missing, or issue an
error depending on what caller wants. oidvectortypes() needs to be
modified as well. I would suggest to change this interface as a
refactoring patch.
4) GetForeignServer and GetForeignDataWrapper need to have a
missing_ok. I suggest to refactor them as well with a separate patch.
5) For operators, there is format_operator_internal which has its own
idea of invalid objects. I think that the existing API should be
reworked.

So, while the work of this thread is largely possible and can be done
incrementally. The devil is in the details of how to handle the
existing APIs. Reaching an agreement about all the points above is key
to get a clean implementation we are happy with.
--
Michael