Cache lookup errors with functions manipulation object addresses

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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.fsf@..., 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Robert Haas
On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
<[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Wed, Jul 19, 2017 at 7:29 PM, Robert Haas <[hidden email]> wrote:
> On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
> <[hidden email]> 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


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

objaddr_nullness_v1.patch.gz (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Alvaro Herrera-9
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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
<[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Alvaro Herrera-9
Michael Paquier wrote:

> On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
> <[hidden email]> 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 Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Thu, Jul 20, 2017 at 6:26 PM, Alvaro Herrera
<[hidden email]> wrote:

> Michael Paquier wrote:
>> On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
>> <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Fri, Jul 21, 2017 at 8:53 AM, Michael Paquier
<[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Thu, Aug 3, 2017 at 7:23 PM, Michael Paquier
<[hidden email]> wrote:

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


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

0001-Refactor-syscache-routines-to-get-attribute-name.patch (16K) Download Attachment
0002-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch (19K) Download Attachment
0003-Refactor-routines-for-subscription-and-publication-l.patch (7K) Download Attachment
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch (163K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Aleksander Alekseeev
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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Wed, Aug 9, 2017 at 2:47 PM, Aleksander Alekseev
<[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Thu, Aug 10, 2017 at 9:48 AM, Michael Paquier
<[hidden email]> wrote:

> On Wed, Aug 9, 2017 at 2:47 PM, Aleksander Alekseev
> <[hidden email]> 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.
Attached is a rebased patch set. Álvaro, as you have introduced most
of the problems with 4464303 & friends dated of 2015 when you
introduced get_object_address(), could you look at this patch set?
--
Michael

0001-Refactor-syscache-routines-to-get-attribute-name.patch (16K) Download Attachment
0002-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch (19K) Download Attachment
0003-Refactor-routines-for-subscription-and-publication-l.patch (7K) Download Attachment
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch (162K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier
<[hidden email]> wrote:
> Attached is a rebased patch set. Álvaro, as you have introduced most
> of the problems with 4464303 & friends dated of 2015 when you
> introduced get_object_address(), could you look at this patch set?

Moved to next commit fest.
--
Michael

Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Thomas Munro-3
On Mon, Nov 27, 2017 at 1:01 AM, Michael Paquier
<[hidden email]> wrote:
> On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier
> <[hidden email]> wrote:
>> Attached is a rebased patch set. Álvaro, as you have introduced most
>> of the problems with 4464303 & friends dated of 2015 when you
>> introduced get_object_address(), could you look at this patch set?

Hi Michael,

FYI:

func.sgml:17550: parser error : Opening and ending tag mismatch:
literal line 17550 and unparseable
   <literal>NULL</> values for undefined objects.
                   ^
func.sgml:17567: parser error : Opening and ending tag mismatch:
literal line 17567 and unparseable
   with <literal>NULL</> values.
                        ^
func.sgml:17582: parser error : Opening and ending tag mismatch:
literal line 17582 and unparseable
   Undefined objects are identified with <literal>NULL</> values.
                                                         ^
func.sgml:20721: parser error : chunk is not well balanced
postgres.sgml:105: parser error : Failure to process entity func
  &func;
        ^
postgres.sgml:105: parser error : Entity 'func' not defined
  &func;
        ^

--
Thomas Munro
http://www.enterprisedb.com

Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Fri, Jan 12, 2018 at 01:45:19PM +1300, Thomas Munro wrote:

> FYI:
>
> func.sgml:17550: parser error : Opening and ending tag mismatch:
> literal line 17550 and unparseable
>    <literal>NULL</> values for undefined objects.
>                    ^
> func.sgml:17567: parser error : Opening and ending tag mismatch:
> literal line 17567 and unparseable
>    with <literal>NULL</> values.
>                         ^
> func.sgml:17582: parser error : Opening and ending tag mismatch:
> literal line 17582 and unparseable
>    Undefined objects are identified with <literal>NULL</> values.
>                                                          ^
> func.sgml:20721: parser error : chunk is not well balanced
> postgres.sgml:105: parser error : Failure to process entity func
>   &func;
>         ^
> postgres.sgml:105: parser error : Entity 'func' not defined
>   &func;
>         ^
Thanks Mr. Robot and Thomas for the reminder. Attached is an updated
patch set taking care of those warnings, 0002 and 0004 being impacted.
--
Michael

0001-Refactor-syscache-routines-to-get-attribute-name.patch (12K) Download Attachment
0002-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch (15K) Download Attachment
0003-Refactor-routines-for-subscription-and-publication-l.patch (5K) Download Attachment
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch (124K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier
On Fri, Jan 12, 2018 at 11:01:59AM +0900, Michael Paquier wrote:
> Thanks Mr. Robot and Thomas for the reminder. Attached is an updated
> patch set taking care of those warnings, 0002 and 0004 being impacted.

The last patch set has rotten enough for git am to complain (not patch
-p1), so attached is a new set.  Alvaro, I am aware that there are way
more shiny features than this patch set, still are you planning to look
at this patch set aimed at doing the cleanup of the existing inferface
you introduced?

I am moving it to the next CF for now..
--
Michael

0001-Refactor-syscache-routines-to-get-attribute-name.patch (12K) Download Attachment
0002-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch (15K) Download Attachment
0003-Refactor-routines-for-subscription-and-publication-l.patch (5K) Download Attachment
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch (124K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Álvaro Herrera
Michael Paquier wrote:
> On Fri, Jan 12, 2018 at 11:01:59AM +0900, Michael Paquier wrote:
> > Thanks Mr. Robot and Thomas for the reminder. Attached is an updated
> > patch set taking care of those warnings, 0002 and 0004 being impacted.
>
> The last patch set has rotten enough for git am to complain (not patch
> -p1), so attached is a new set.

Pushed 0001, which was easy enough to deal with.  I think 0002 and 0003
should be changed similarly: the elog(ERROR) code should be inside "if"
and the "return NULL" case the straight path, rather than the other way
around.  That seems more robust than the compiler relying on knowledge
that elog(ERROR) does not return.

As far as format_type_extended() is concerned, IMO we've gone far enough
with the number of variants of format_type().  Making the function
public makes sense to me, but let's add a bits32 flags argument instead
of exposing the messy set of booleans.  We can add compatibility
wrappers for the flag combinations most used in core code, and maybe
take the opportunity phase out the uncommon ones.

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

Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier-2
On Mon, Feb 12, 2018 at 07:57:34PM -0300, Alvaro Herrera wrote:
> Pushed 0001, which was easy enough to deal with.

Thanks.

> I think 0002 and 0003 should be changed similarly: the elog(ERROR)
> code should be inside "if" and the "return NULL" case the straight
> path, rather than the other way around.  That seems more robust than
> the compiler relying on knowledge that elog(ERROR) does not return.

OK, I updated the patches to do so and rebased.  Those are now 0001 and
0002.  For 0002, I have added more adapted comments at the top of
get_publication_name and get_subscription_name.

> As far as format_type_extended() is concerned, IMO we've gone far enough
> with the number of variants of format_type().  Making the function
> public makes sense to me, but let's add a bits32 flags argument instead
> of exposing the messy set of booleans.  We can add compatibility
> wrappers for the flag combinations most used in core code, and maybe
> take the opportunity phase out the uncommon ones.

OK, I was a bit hesitant to propose that without more input, so I
definitely agree with this API interface.  I have tackled that in 0003,
with the following changes:
- let's get rid of format_type_with_typemod_qualified.  This is only
used by postgres_fdw in one place.
- format_type_be_qualified is also rather localized, but I have kept
it.  Perhaps this could be nuked as well.  Input is welcome.
- let's keep format_type_be and format_type_with_typemod.  Those are
largely more spread in the core code, so I don't think that we need to
invade things more than necessary.

Attached is a rebased and updated patch set.  I have also reworked the
dance with elog calls and missing_ok to match with what you have already
committed.
--
Michael

0001-Extend-lookup-routines-for-FDW-and-foreign-server-wi.patch (15K) Download Attachment
0002-Refactor-routines-for-subscription-and-publication-l.patch (6K) Download Attachment
0003-Refactor-format_type-APIs-to-be-more-modular.patch (8K) Download Attachment
0004-Eliminate-user-visible-cache-lookup-errors-for-objad.patch (124K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Álvaro Herrera
Michael Paquier wrote:

> > As far as format_type_extended() is concerned, IMO we've gone far enough
> > with the number of variants of format_type().  Making the function
> > public makes sense to me, but let's add a bits32 flags argument instead
> > of exposing the messy set of booleans.  We can add compatibility
> > wrappers for the flag combinations most used in core code, and maybe
> > take the opportunity phase out the uncommon ones.
>
> OK, I was a bit hesitant to propose that without more input, so I
> definitely agree with this API interface.  I have tackled that in 0003,
> with the following changes:
> - let's get rid of format_type_with_typemod_qualified.  This is only
> used by postgres_fdw in one place.
> - format_type_be_qualified is also rather localized, but I have kept
> it.  Perhaps this could be nuked as well.  Input is welcome.
> - let's keep format_type_be and format_type_with_typemod.  Those are
> largely more spread in the core code, so I don't think that we need to
> invade things more than necessary.

Pushed 0003.  Maybe we can get rid of format_type_be_qualified too, but
I didn't care too much about it either; it's not a big deal I think.

What interested me more was whether we could get rid of the
FORMAT_TYPE_TYPEMOD_GIVEN flag, but ended up deciding not to pursue that
as a phenomenal waste of time.  Here are some references in case you
care.

https://www.postgresql.org/message-id/flat/200111101659.fAAGxKX06044%40postgresql.org
https://git.postgresql.org/pg/commitdiff/a585c20d12d0e22befc8308e9f8ccb6f54a5df69

Thanks

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

Reply | Threaded
Open this post in threaded view
|

Re: Cache lookup errors with functions manipulation object addresses

Michael Paquier-2
On Sat, Feb 17, 2018 at 07:17:24PM -0300, Alvaro Herrera wrote:
> Pushed 0003.

Thanks.

> Maybe we can get rid of format_type_be_qualified too, but I didn't
> care too much about it either; it's not a big deal I think.

Agreed. There are 16 callers spread in objectaddress.c and regproc.c,
this would generate some diffs.  If there are extra opinions later on,
we could always revisit that.  The new API is modular enough anyway.

> What interested me more was whether we could get rid of the
> FORMAT_TYPE_TYPEMOD_GIVEN flag, but ended up deciding not to pursue that
> as a phenomenal waste of time.  Here are some references in case you
> care.
>
> https://www.postgresql.org/message-id/flat/200111101659.fAAGxKX06044%40postgresql.org
> https://git.postgresql.org/pg/commitdiff/a585c20d12d0e22befc8308e9f8ccb6f54a5df69

Thanks for the threads, I didn't know about them.  I thought as well
about trying to remove FORMAT_TYPE_TYPEMOD_GIVEN but avoided to do so,
so as not to break things the way they should be for a long time as this
code is as it is now for at least as long as I am working on Postgres.
I didn't check the git history to see the logic behind the code though,
which I really should have done.  So thanks for the references.
--
Michael

signature.asc (849 bytes) Download Attachment
12