Re: Cache lookup errors with functions manipulation object addresses

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Moshe Jacobson <moshe(at)neadwerx(dot)com>
Subject: Re: Cache lookup errors with functions manipulation object addresses
Date: 2017-08-04 14:27:17
Message-ID: CAB7nPqQH6hOxy1GNv6ZrDEqxWoxM5qq2LhvZB+5Pto=adJrCug@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Refactor-syscache-routines-to-get-attribute-name.patch application/octet-stream 12.1 KB
0002-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch application/octet-stream 14.3 KB
0003-Refactor-routines-for-subscription-and-publication-l.patch application/octet-stream 5.7 KB
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch application/octet-stream 118.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-08-04 15:50:16 Re: pg_upgrade failed with error - ERROR: column "a" in child table must be marked NOT NULL
Previous Message Robert Haas 2017-08-04 14:18:45 Re: Default Partition for Range