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

[Michael Paquier]

On Wed, Aug 9, 2017 at 2:47 PM, Aleksander Alekseev
 wrote:
> I believe this patch is "Ready for Committer".
>
> The new status of this patch is: Ready for Committer

Thanks for the lookup, but I think that this is still hasty as no
discussion has happened about a couple of APIs to get object names.
Types, operators and functions have no "cache lookup" and prefer
producing names like "???" or "-". We may want to change that. Or not.
The current patch keeps existing interfaces as much as possible but
those existing caveats remain.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Aleksander Alekseev]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

I believe this patch is "Ready for Committer".

The new status of this patch is: Ready for Committer

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Fri, Jul 21, 2017 at 8:53 AM, Michael Paquier
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Thu, Jul 20, 2017 at 6:26 PM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>> On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
>>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Alvaro Herrera]

Michael Paquier wrote:
> On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
>  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.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
 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.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Alvaro Herrera]

Michael Paquier wrote:

> - getObjectDescription and getObjectIdentity are called in quite a
> couple of places. We could have those have a kind of missing_ok, but
> as the status is just for adding cache lookup errors I have kept the
> interface simple as this keeps the code in objectaddress.c more simple
> as well. getObjectIdentity is used mainly in sepgsql, which I have not
> compiled yet so I may have missed something :) getObjectDescription is
> used in more places in the backend code, but I am not much into
> complicating the objaddr API with this patch more.

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.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Michael Paquier]

On Wed, Jul 19, 2017 at 7:29 PM, Robert Haas  wrote:
> On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
>  wrote:
>> Would we want to improve the error handling of such objects?
>
> +1 for such an improvement.

Attached is a patch for all that. Here are some notes:
- format_type_be and friends use allow_invalid. I have added a flag to
control the NULL-ness as many code paths rely on existing APIs, and
introduced an _extended version of this API. I would argue for the
removal of allow_invalid to give more flexibility to callers, but this
impacts extensions :(
- A similar thing is needed for format_operator().
- We could really add a missing_ok to get_attname, but that does not
seem worth the refactoring with modules potentially calling it..
- GetForeignDataWrapper is extended with a missing_ok, unfortunately
not saving one cache lookup in GetForeignDataWrapperByName.
- Same remark as the previous one for GetForeignServer.
- get_publication_name and get_subscription_name gain a missing_ok.
- getObjectDescription and getObjectIdentity are called in quite a
couple of places. We could have those have a kind of missing_ok, but
as the status is just for adding cache lookup errors I have kept the
interface simple as this keeps the code in objectaddress.c more simple
as well. getObjectIdentity is used mainly in sepgsql, which I have not
compiled yet so I may have missed something :) getObjectDescription is
used in more places in the backend code, but I am not much into
complicating the objaddr API with this patch more.
- I have added tests for all the OCLASS objects, for a total more or
less 120 cache lookup errors that a user can face.
- Some docs are present as well, but I think that they are a bit
incomplete. I'll review them a bit later.
- The patch is large, 800 lines are used for the tests which is very mechanical:
 32 files changed, 1721 insertions(+), 452 deletions(-)

Thanks,
-- 
Michael


objaddr_nullness_v1.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Robert Haas]

On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
 wrote:
> Would we want to improve the error handling of such objects?

+1 for such an improvement.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

Hi all,

Per an offline report from Moshe Jacobson, it is possible to trigger
easily cache lookup errors using pg_describe_object with invalid
object IDs and pg_describe_object(). I had a closer look at things in
this area, to notice that there are other user-facing failures as many
things use the same interface:
=# select pg_identify_object('pg_class'::regclass, 0::oid, 0);
ERROR:  XX000: cache lookup failed for relation 0
=# select pg_describe_object('pg_class'::regclass, 0::oid, 0);
ERROR:  XX000: cache lookup failed for relation 0
=# select pg_identify_object_as_address('pg_class'::regclass, 0::oid, 0);
ERROR:  XX000: cache lookup failed for relation 0

As my previous opinion on the matter in
https://www.postgresql.org/message-id/87wpxfygg9@credativ.de, I
still think that "cache lookup" failures should not be things a user
is able to trigger at will, and that those errors should be replaced
by proper NULL results. That's clearly not an item for PG10, but we
could consider improving things for PG11. Still, we are talking about
adding NULLness handling in getObjectDescription(), which goes into
low-level functions to grab the name of some objects, and some of
those functions have their own way to deal with incorrect objects
(format_type_be returns "???" for example for functions).

Would we want to improve the error handling of such objects? Or that's
not worth the effort? Álvaro, what's your take on the matter as you
worked a lot on that?

Thoughts,
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers