Re: [HACKERS] Cache lookup errors with functions manipulation object addresses

On Fri, Jul 21, 2017 at 8:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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.

Thinking more on that, I'll go with the flag Alvaro suggested.

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

format_procedure_qualified is only used for objaddr.c, so I am going
here for not defining a compatibility set of macros.

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

But here get_relid_attribute_name is old enough to have a
compatibility macro. So I'll add one in one of the refactoring
patches.

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

With compatibility macros.

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

No convinced much here, format_operator_qualified is not widely used
as far as I see, so I would just replace it with the _extended
version.

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

Extra opinions always welcome.
-- 
Michael