Cache lookup errors with functions manipulation object addresses

On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
<michael.paquier@gmail.com> 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

On Wed, Jul 19, 2017 at 7:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jul 19, 2017 at 2:25 AM, Michael Paquier
<michael.paquier@gmail.com> 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

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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

Michael Paquier wrote:

On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jul 20, 2017 at 6:26 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

On Thu, Jul 20, 2017 at 4:04 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> 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

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

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

On Thu, Aug 3, 2017 at 7:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

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

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

On Wed, Aug 9, 2017 at 2:47 PM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> 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

On Thu, Aug 10, 2017 at 9:48 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Wed, Aug 9, 2017 at 2:47 PM, Aleksander Alekseev
<a.alekseev@postgrespro.ru> 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

On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier
<michael.paquier@gmail.com> 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

On Mon, Nov 27, 2017 at 1:01 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Fri, Nov 24, 2017 at 9:13 PM, Michael Paquier
<michael.paquier@gmail.com> 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

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

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

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

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

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.

/messages/by-id/200111101659.fAAGxKX06044@postgresql.org
https://git.postgresql.org/pg/commitdiff/a585c20d12d0e22befc8308e9f8ccb6f54a5df69

Thanks

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

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.

/messages/by-id/200111101659.fAAGxKX06044@postgresql.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