Re: Cache lookup errors with functions manipulation object addresses

On Thu, Aug 3, 2017 at 7:23 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Jul 21, 2017 at 8:53 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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.

This part is done. All the existing functions working in objectaddress
gain a missing_ok argument. The SQL-callable functions allow undefined
objects, and backend-side callers fail as before.

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

While hacking on it again, I have again changed my mind to keep the
patch simple. On error, format_procedure and format_operator return
the operator numerically. The attached set of patches does not change
that.

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

Here I have changed only get_attname signature and removed
get_relid_attribute_name() as any combination change would result in a
compilation failure.

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

Here as well, I have decided to keep the patch simple, and use the
existing flag allow_invalid as an equivalent for missing_ok. Similarly
to procedures and operators, we could always reinvent the wheel with
an extra set of routines... So extra opinions are welcome.

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

Here also I have finished with an unchanged interface as
format_operator_internal returns no errors.

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

A set of patches easier to digest is attached:
- 0001 refactors things for attribute names.
- 0002 refactors FDW and foreign servers.
- 0003 refactors publications and subscriptions.
- 0004 is the main patch changing object address interface to avoid
cache lookup failures.
--
Michael