Re: [HACKERS] Cache lookup errors with functions manipulation object addresses
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
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
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
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
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
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
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
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
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
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