Обсуждение: [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.fsf@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

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

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

-- 
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:

> - 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

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

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

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

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

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

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

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

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

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.

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

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

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

Hello Michael,

It looks like that this patch doesn't apply anymore: http://commitfest.cputube.org/

The new status of this patch is: Waiting on Author

On Mon, Mar 05, 2018 at 12:57:38PM +0000, Aleksander Alekseev wrote:
> It looks like that this patch doesn't apply anymore: http://commitfest.cputube.org/
>
> The new status of this patch is: Waiting on Author

Yes, this happens because patch 0003 from the last series has been
committed as a26116c6.  Attached is a rebased set, though the patches
have no actual changes as those are able to apply correctly.
--
Michael

On 2018-Mar-06, Michael Paquier wrote:

> On Mon, Mar 05, 2018 at 12:57:38PM +0000, Aleksander Alekseev wrote:
> > It looks like that this patch doesn't apply anymore: http://commitfest.cputube.org/
> > 
> > The new status of this patch is: Waiting on Author
> 
> Yes, this happens because patch 0003 from the last series has been
> committed as a26116c6.  Attached is a rebased set, though the patches
> have no actual changes as those are able to apply correctly.

I forgot to mention this earlier: I think external FDWs do not really
benefit from your 0001 (neither do most of the internal callsites of the
modified functions).  I checked a bunch of source files and nowhere I
got a "Gee, I wish this returned InvalidOid if the object in question
didn't exist" feeling.

https://github.com/Kozea/Multicorn/blob/master/src/multicorn.c
https://github.com/tds-fdw/tds_fdw/blob/master/src/tds_fdw.c
https://github.com/pramsey/pgsql-ogr-fdw/blob/master/ogr_fdw.c
https://github.com/EnterpriseDB/mysql_fdw/blob/master/mysql_fdw.c
https://github.com/laurenz/oracle_fdw/blob/master/oracle_fdw.c

I think we're better off adding a new function and avoid changing the
signature of GetForeignServer et al.  Or maybe rename the function and
keep the original name as a compatibility wrapper macro.

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

On Mon, May 14, 2018 at 04:27:48PM -0400, Alvaro Herrera wrote:
> I think we're better off adding a new function and avoid changing the
> signature of GetForeignServer et al.  Or maybe rename the function and
> keep the original name as a compatibility wrapper macro.

On the other hand, if we make the change visible because of a
compilation failures, then modules would become aware of the problem and
react?  I would not expect modules to set missing_ok to true anyway as
they expect those objects to exist, so I can live with a new function.
What about naming those GetForeignServerExtended and
GetForeignDataWrapperExtended?
--
Michael

On 2018-May-15, Michael Paquier wrote:

> On Mon, May 14, 2018 at 04:27:48PM -0400, Alvaro Herrera wrote:
> > I think we're better off adding a new function and avoid changing the
> > signature of GetForeignServer et al.  Or maybe rename the function and
> > keep the original name as a compatibility wrapper macro.
> 
> On the other hand, if we make the change visible because of a
> compilation failures, then modules would become aware of the problem and
> react?

Presumably, if you invoke a FDW and its handler finds
that the ForeignServer doesn't exist, what is it to do other than raise
an error?  I can't see that there's any possible improvement.

So, I don't know -- if the reaction is to add a #ifdef for pg version
that adds a second argument passed always false, then we haven't won
anything.

> I would not expect modules to set missing_ok to true anyway as they
> expect those objects to exist, so I can live with a new function.

Exactly.

> What about naming those GetForeignServerExtended and
> GetForeignDataWrapperExtended?

WFM.

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

On Mon, May 14, 2018 at 06:32:52PM -0400, Alvaro Herrera wrote:
> So, I don't know -- if the reaction is to add a #ifdef for pg version
> that adds a second argument passed always false, then we haven't won
> anything.

Yeah, that improves nothing..

>> What about naming those GetForeignServerExtended and
>> GetForeignDataWrapperExtended?
>
> WFM.

Okay, I have done so in the updated set attached.  I have added some
documentation as well in fdwhandler.sgml about those two new things.
That's too late for v11 of course, so let's them sit until the time
comes.
--
Michael

On Wed, May 16, 2018 at 01:03:18PM +0900, Michael Paquier wrote:
> Okay, I have done so in the updated set attached.  I have added some
> documentation as well in fdwhandler.sgml about those two new things.
> That's too late for v11 of course, so let's them sit until the time
> comes.

Attached are refreshed versions for this commit fest.  The deal for this
commit fest is to not consider large patches this time, so please let me
suggest to discard 0003 even if it is very mechanical.  I would like
however to get 0001 and 0002 merged during this commit fest so as we can
move on with this thread as those are simple changes, and finish 0003
afterwards.
--
Michael

On 07/01/2018 10:27 AM, Michael Paquier wrote:
> On Wed, May 16, 2018 at 01:03:18PM +0900, Michael Paquier wrote:
>> Okay, I have done so in the updated set attached.  I have added some
>> documentation as well in fdwhandler.sgml about those two new things.
>> That's too late for v11 of course, so let's them sit until the time
>> comes.
> Attached are refreshed versions for this commit fest.  The deal for this
> commit fest is to not consider large patches this time, so please let me
> suggest to discard 0003 even if it is very mechanical.  I would like
> however to get 0001 and 0002 merged during this commit fest so as we can
> move on with this thread as those are simple changes, and finish 0003
> afterwards.


I think you're asserting far too broad a policy for the CF, and in any 
case there has been no discussion of what exactly is a large patch. I 
don't see any great need to defer patch 3. It is substantial although 
not what I would class as large, but it also has relatively low impact, 
ISTM.

cheers

andrew

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

On Sun, Jul 01, 2018 at 12:31:17PM -0400, Andrew Dunstan wrote:
> I think you're asserting far too broad a policy for the CF, and in any case
> there has been no discussion of what exactly is a large patch. I don't see
> any great need to defer patch 3. It is substantial although not what I would
> class as large, but it also has relatively low impact, ISTM.

I am fine with any conclusion.  As the patch has rotten a bit, I
switched it from "Ready for committer" to "Needs Review" by the way.
That seems more appropriate as the thing has rotten a bit.
--
Michael

On Mon, Jul 02, 2018 at 08:54:25PM +0900, Michael Paquier wrote:
> I am fine with any conclusion.  As the patch has rotten a bit, I
> switched it from "Ready for committer" to "Needs Review" by the way.
> That seems more appropriate as the thing has rotten a bit.

Attached are rebased versions.  This has been around for some time, so I
am planning to move on with this patch set pretty soon as that's mainly
cleanup for getObjectIdentity as it triggers elog("cache lookup") or
such for undefined objects.  Patch 0001 extends FDW and server routines
so as it is possible to skip missing entries, without breaking
compatibility.  Patch 0002 adds a missing_ok flag when doing
subscription and publication lookups.

Any objections?
--
Michael

On Fri, Sep 14, 2018 at 11:22:12AM +0900, Michael Paquier wrote:
> Attached are rebased versions.  This has been around for some time, so I
> am planning to move on with this patch set pretty soon as that's mainly
> cleanup for getObjectIdentity as it triggers elog("cache lookup") or
> such for undefined objects.  Patch 0001 extends FDW and server routines
> so as it is possible to skip missing entries, without breaking
> compatibility.  Patch 0002 adds a missing_ok flag when doing
> subscription and publication lookups.
>
> Any objections?

And I forgot to attach the patches..
--
Michael

On 2018-Sep-14, Michael Paquier wrote:

> On Fri, Sep 14, 2018 at 11:22:12AM +0900, Michael Paquier wrote:
> > Attached are rebased versions.  This has been around for some time, so I
> > am planning to move on with this patch set pretty soon as that's mainly
> > cleanup for getObjectIdentity as it triggers elog("cache lookup") or
> > such for undefined objects.  Patch 0001 extends FDW and server routines
> > so as it is possible to skip missing entries, without breaking
> > compatibility.  Patch 0002 adds a missing_ok flag when doing
> > subscription and publication lookups.
> > 
> > Any objections?
> 
> And I forgot to attach the patches..

Patches 0001 and 0002 look OK to me now, on a cursory glance.

I haven't looked at 0003 yet.

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

On 2018-Sep-14, Alvaro Herrera wrote:

> I haven't looked at 0003 yet.

It's strange that pg_identify_object returns empty type in only some
cases (as seen in the regression test output) ... and this one
definitely does not make sense:

+SELECT * FROM pg_identify_object('pg_class'::regclass, 'pg_class'::regclass, -8); -- no column for relation
+     type     |   schema   |   name   |      identity       
+--------------+------------+----------+---------------------
+ table column | pg_catalog | pg_class | pg_catalog.pg_class
+(1 row)

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

On Fri, Sep 14, 2018 at 12:07:23PM -0300, Alvaro Herrera wrote:
> On 2018-Sep-14, Alvaro Herrera wrote:
>> I haven't looked at 0003 yet.

Thanks for the review.  I have pushed 0002 for now.  I had one doubt
about 0001 though.  So as to avoid redesigning the APIs for FDWs and
servers again in the future, wouldn't we want to give them a uint32
flags which can be in with more than one option.  There would be only
one option for now, which is what I have done in the attached.

> It's strange that pg_identify_object returns empty type in only some
> cases (as seen in the regression test output) ...

Are you referring to something in particular?  All the routines used in
objectaddress.c are part of what exists in core for some time now,
particularly handling for operators, functions and types has been sort
of messy.  It is of course possible to reach a state in the code where
we have options to get NULL results for all those things.  For example,
format_procedure_internal could be refactored in a way similar to what
happens for format_type_extended.  And format_type_extended could gain a
FORCE_NULL flag which makes sure that on an undefined object
objectaddress.c sees a NULL object.  That was one of the points raised
upthread that I still wanted to discuss..  One thing is that I am not
sure if it is worth complicating the existing interface even more just
to deal with undefined objects as pure NULLs.  Thoughts are welcome.

> And this one definitely does not make sense:
>
> +SELECT * FROM pg_identify_object('pg_class'::regclass, 'pg_class'::regclass, -8); -- no column for relation
> +     type     |   schema   |   name   |      identity
> +--------------+------------+----------+---------------------
> + table column | pg_catalog | pg_class | pg_catalog.pg_class
> +(1 row)

Indeed, this one is not intentional, and can be easily addressed by
checking first for the attribute existence.  This is corrected in the
attached version.
--
Michael

On 2018-Sep-18, Michael Paquier wrote:

> On Fri, Sep 14, 2018 at 12:07:23PM -0300, Alvaro Herrera wrote:
> > On 2018-Sep-14, Alvaro Herrera wrote:
> >> I haven't looked at 0003 yet.
> 
> Thanks for the review.  I have pushed 0002 for now.  I had one doubt
> about 0001 though.  So as to avoid redesigning the APIs for FDWs and
> servers again in the future, wouldn't we want to give them a uint32
> flags which can be in with more than one option.  There would be only
> one option for now, which is what I have done in the attached.

Agreed.

I think defining bit flags in an enum is weird; I'd use defines instead.
And (a purely stylistic point) I'd use bits32 rather than uint32.  (I'm
probably alone in this opinion, since almost every interface passing
flags uses unsigned int of some size.  But "bits" types are defined
precisely for this purpose!) Compare a61f5ab98638.

I support 0001 other than those two points.

> > It's strange that pg_identify_object returns empty type in only some
> > cases (as seen in the regression test output) ...
> 
> Are you referring to something in particular?

Yeah, these two cases:

+-- Keep those checks in the same order as getObjectTypeDescription()
+SELECT * FROM pg_identify_object_as_address('pg_class'::regclass, 0, 0); -- no relation
+ type | object_names | object_args 
+------+--------------+-------------
+      |              | 
+(1 row)

+SELECT * FROM pg_identify_object_as_address('pg_proc'::regclass, 0, 0); -- no function
+ type | object_names | object_args 
+------+--------------+-------------
+      | {}           | {}
+(1 row)

All the other cases you add have a non-empty value in "type".

> All the routines used in objectaddress.c are part of what exists in
> core for some time now, particularly handling for operators, functions
> and types has been sort of messy.

I think this is our chance to fix the mess.  Previously (before I added
the SQL-access of the object addressing mechanism in 9.5 I mean) it
wasn't possible to get at this code at all with arbitrary input, so this
is all a "new" problem (I mean new in the last 5 years).

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

On Wed, Dec 12, 2018 at 02:21:29PM -0300, Alvaro Herrera wrote:
> I think defining bit flags in an enum is weird; I'd use defines
> instead.

Okay, I am fine with that.

> And (a purely stylistic point) I'd use bits32 rather than uint32.  (I'm
> probably alone in this opinion, since almost every interface passing
> flags uses unsigned int of some size.  But "bits" types are defined
> precisely for this purpose!) Compare a61f5ab98638.

Fine with that as well, let's do as you suggest then.

> I support 0001 other than those two points.

Attached is an updated version for that as 0001.  Thanks for the
review.  Does that part look good to you now?

> Yeah, these two cases:
>
> +-- Keep those checks in the same order as getObjectTypeDescription()
> +SELECT * FROM pg_identify_object_as_address('pg_class'::regclass, 0, 0); -- no relation
> + type | object_names | object_args
> +------+--------------+-------------
> +      |              |
> +(1 row)

"type" should show "relation" here, yes.

> +SELECT * FROM pg_identify_object_as_address('pg_proc'::regclass, 0, 0); -- no function
> + type | object_names | object_args
> +------+--------------+-------------
> +      | {}           | {}
> +(1 row)
>
> All the other cases you add have a non-empty value in "type".

On this one I would expect NULL for object_names and object_args, as
empty does not make sense for an undefined object, still "type" should
print...  "type".

+SELECT * FROM pg_identify_object_as_address('pg_constraint'::regclass, 0, 0); -- no constraint
+ type | object_names | object_args
+------+--------------+-------------
+      |              |
+(1 row)
Constraints also are empty.

+SELECT * FROM pg_identify_object_as_address('pg_largeobject'::regclass, 0, 0); -- no large object, no error
+     type     | object_names | object_args
+--------------+--------------+-------------
+ large object | {0}          | {}
+(1 row)
Large objects should return NULL for the last two fields.

There are a couple of other inconsistent cases with the existing sub-APIs:
+SELECT * FROM pg_identify_object_as_address('pg_type'::regclass, 0, 0); -- no type
+ type | object_names | object_args
+------+--------------+-------------
+ type | {-}          | {}
+(1 row)
Here I would expect NULL for object_names and object_args.

> I think this is our chance to fix the mess.  Previously (before I added
> the SQL-access of the object addressing mechanism in 9.5 I mean) it
> wasn't possible to get at this code at all with arbitrary input, so this
> is all a "new" problem (I mean new in the last 5 years).

This requires a bit more work with the low-level APIs grabbing the
printed information though.  And I think that the following guidelines
make sense to work on as a base definition for undefined objects:
- pg_identify_object returns the object type name, NULL for the other fields.
- pg_describe_object returns just NULL.
- pg_identify_object_as_address returns the object type name and NULL
for the other fields.

There is some more refactoring work still needed for constraints, large
objects and functions, in a way similar to a26116c6.  I am pretty happy
with the shape of 0001, so this could be applied, 0002 still needs to be
reworked so as all undefined object types behave as described above in a
consistent manner.  Do those definitions make sense?
--
Michael

On 2018-Dec-13, Michael Paquier wrote:

> > I support 0001 other than those two points.
> 
> Attached is an updated version for that as 0001.  Thanks for the
> review.  Does that part look good to you now?

+1.

> > +SELECT * FROM pg_identify_object_as_address('pg_proc'::regclass, 0, 0); -- no function
> > + type | object_names | object_args 
> > +------+--------------+-------------
> > +      | {}           | {}
> > +(1 row)
> > 
> > All the other cases you add have a non-empty value in "type".
> 
> On this one I would expect NULL for object_names and object_args, as
> empty does not make sense for an undefined object, still "type" should
> print...  "type".

Hmm ... "routine"?

I'm not sure if NULLs are better than empty arrays, but I agree that we
should pick one representation for undefined object and use it
consistently for all object types.

> > I think this is our chance to fix the mess.  Previously (before I added
> > the SQL-access of the object addressing mechanism in 9.5 I mean) it
> > wasn't possible to get at this code at all with arbitrary input, so this
> > is all a "new" problem (I mean new in the last 5 years).

Obviously I failed to subtract 11 from 9.5 correctly ... I meant 4
years.


> This requires a bit more work with the low-level APIs grabbing the
> printed information though.  And I think that the following guidelines
> make sense to work on as a base definition for undefined objects:
> - pg_identify_object returns the object type name, NULL for the other fields.
> - pg_describe_object returns just NULL.
> - pg_identify_object_as_address returns the object type name and NULL
> for the other fields.

Sounds good to me.

> There is some more refactoring work still needed for constraints, large
> objects and functions, in a way similar to a26116c6.  I am pretty happy
> with the shape of 0001, so this could be applied, 0002 still needs to be
> reworked so as all undefined object types behave as described above in a
> consistent manner.  Do those definitions make sense?

I think so, yes.

Thanks for taking care of this.

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

On Thu, Dec 13, 2018 at 02:58:02PM -0300, Alvaro Herrera wrote:
> On 2018-Dec-13, Michael Paquier wrote:
>> Attached is an updated version for that as 0001.  Thanks for the
>> review.  Does that part look good to you now?
>
> +1.

Thanks for the review, I have applied this part.

> Hmm ... "routine"?

That's even better.

> I'm not sure if NULLs are better than empty arrays, but I agree that we
> should pick one representation for undefined object and use it
> consistently for all object types.

Okay, thanks.

>> There is some more refactoring work still needed for constraints, large
>> objects and functions, in a way similar to a26116c6.  I am pretty happy
>> with the shape of 0001, so this could be applied, 0002 still needs to be
>> reworked so as all undefined object types behave as described above in a
>> consistent manner.  Do those definitions make sense?
>
> I think so, yes.
>
> Thanks for taking care of this.

Thanks again for looking up at what was proposed.  I'll see if I can
finish the refactoring part for the next CF, and be done with this
stuff.
--
Michael

Álvaro, all,

On Fri, Dec 14, 2018 at 09:04:36AM +0900, Michael Paquier wrote:
> Thanks again for looking up at what was proposed.  I'll see if I can
> finish the refactoring part for the next CF, and be done with this
> stuff.

Please find attached an updated patch set which reworks the SQL
interface for object addresses so as NULL-ness is handled consistently
for all object types.  This has needed a couple of extensions to some
current object-lookup APIs, which are added as refactoring bits
without breaking existing callers:
- Procedures and operators gain a new _extended routine which can take
a set of flags to enforce a NULL result or force qualification.
- format_type_extended needs an extra flag to enforce NULL.

On top of the refactoring issues, here is the list of changes to make
things work:
- The object type can never be NULL, meaning that for undefined
relations, routines or constraints the caller gets a generic name even
if the object is not defined.  (That's the definition mentioned a
couple of messages upthread).
- Large object existence is checked with LargeObjectExists().
- Table columns show all the fields as NULL if the object identity
cannot be found instead of showing the table and its schema.

With that all the regression tests show a consistent behavior for all
object types when those are undefined.  Thoughts?
--
Michael

On Tue, Dec 25, 2018 at 01:34:38PM +0900, Michael Paquier wrote:
> With that all the regression tests show a consistent behavior for all
> object types when those are undefined.  Thoughts?

End-of-commit-fest rebase and moved to next CF.
--
Michael

On Thu, Jan 31, 2019 at 04:08:18PM +0900, Michael Paquier wrote:
> End-of-commit-fest rebase and moved to next CF.

Rebased version fixing some conflicts with HEAD.
--
Michael

On Thu, Feb 21, 2019 at 04:40:13PM +0900, Michael Paquier wrote:
> Rebased version fixing some conflicts with HEAD.

And rebased version for this stuff on HEAD (66c5bd3), giving visibly
v16.
--
Michael

> On Tue, Jul 2, 2019 at 9:28 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 21, 2019 at 04:40:13PM +0900, Michael Paquier wrote:
> > Rebased version fixing some conflicts with HEAD.
>
> And rebased version for this stuff on HEAD (66c5bd3), giving visibly
> v16.

Thanks for the patch! I couldn't check it in action, since looks like it
doesn't apply anymore [1] (although after a quick check I'm not entirely sure
why). Nevertheless I have a few short commentaries:

    v16-0001-Add-flag-to-format_type_extended-to-enforce-NULL.patch

    - if (type_oid == InvalidOid && (flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
    - return pstrdup("-");
    + if (type_oid == InvalidOid)
    + {
    + if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
    + return NULL;
    + else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
    + return pstrdup("-");
    + }

Here and in format_operator_extened commentary says

    * Returns a palloc'd string.

but now it's possible to return NULL, so I guess comments need to be adjusted,
right?

    v16-0003-Eliminate-user-visible-cache-lookup-errors-for-o.patch

    - appendStringInfo(&buffer, _("operator %s"),
    - format_operator(object->objectId));
    - break;
    + {
    + char *oprname = format_operator_extended(object->objectId,
    + FORMAT_PROC_FORCE_NULL);

Shouldn't it be FORMAT_OPERATOR_FORCE_NULL here?

I'll continue reviewing the last patch in more details.

[1]: http://cfbot.cputube.org/patch_24_1947.log

On Mon, Sep 23, 2019 at 09:15:24PM +0200, Dmitry Dolgov wrote:
> Thanks for the patch! I couldn't check it in action, since looks like it
> doesn't apply anymore [1] (although after a quick check I'm not entirely sure
> why). Nevertheless I have a few short commentaries:

Thanks for the review.  There was a small conflict in objectaddress.h
easy enough to solve.

> Here and in format_operator_extened commentary says
>
>     * Returns a palloc'd string.
>
> but now it's possible to return NULL, so I guess comments need to be adjusted,
> right?

Right.

>     v16-0003-Eliminate-user-visible-cache-lookup-errors-for-o.patch
>
>     - appendStringInfo(&buffer, _("operator %s"),
>     - format_operator(object->objectId));
>     - break;
>     + {
>     + char *oprname = format_operator_extended(object->objectId,
>     + FORMAT_PROC_FORCE_NULL);
>
> Shouldn't it be FORMAT_OPERATOR_FORCE_NULL here?

Indeed, that's the case.

Please feel free to use the updated versions attached.  These can
apply on top of HEAD at 30d1379.
--
Michael

At Tue, 24 Sep 2019 08:58:50 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190923235850.GA2012@paquier.xyz>
> On Mon, Sep 23, 2019 at 09:15:24PM +0200, Dmitry Dolgov wrote:
> Please feel free to use the updated versions attached.  These can
> apply on top of HEAD at 30d1379.

In 0003, empty string and NULL are not digtinguishable in psql
text output. It'd be better that the regression test checks that
the return is actually NULL using "is NULL" or "\pset null
'<null>'" or something like.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Sep 24, 2019 at 03:25:08PM +0900, Kyotaro Horiguchi wrote:
> In 0003, empty string and NULL are not digtinguishable in psql
> text output. It'd be better that the regression test checks that
> the return is actually NULL using "is NULL" or "\pset null
> '<null>'" or something like.

For a patch whose purpose is to check after NULL, I missed to consider
that...  Thanks!  I have just added a "\pset null NULL" because that's
less bulky than the other option, and all the results properly show
NULL, without any empty strings around.  I am not sending a new patch
set just for that change though, and the most recent version is here:
https://github.com/michaelpq/postgres/tree/objaddr_nulls
--
Michael

On 2019-Sep-25, Michael Paquier wrote:

> On Tue, Sep 24, 2019 at 03:25:08PM +0900, Kyotaro Horiguchi wrote:
> > In 0003, empty string and NULL are not digtinguishable in psql
> > text output. It'd be better that the regression test checks that
> > the return is actually NULL using "is NULL" or "\pset null
> > '<null>'" or something like.
> 
> For a patch whose purpose is to check after NULL, I missed to consider
> that...  Thanks!  I have just added a "\pset null NULL" because that's
> less bulky than the other option, and all the results properly show
> NULL, without any empty strings around.  I am not sending a new patch
> set just for that change though, and the most recent version is here:
> https://github.com/michaelpq/postgres/tree/objaddr_nulls

I have still to review this comprehensively, but one thing I don't quite
like is the shape of the new regression tests, which seem a bit too
bulky ... why not use the style elsewhere in that file, with a large
VALUES clause to supply input params for a single query?  That would be
a lot faster, too.

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

On 2019-Sep-24, Michael Paquier wrote:

> + * - FORMAT_TYPE_FORCE_NULL
> + *            if the type OID is invalid or unknown, return NULL instead of ???
> + *            or such

I think FORCE_NULL is a strange name for this flag (and its two
siblings); I think something like FORMAT_TYPE_INVALID_AS_NULL is
clearer.

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

On Wed, Sep 25, 2019 at 09:21:03AM -0300, Alvaro Herrera wrote:
> On 2019-Sep-24, Michael Paquier wrote:
>> + * - FORMAT_TYPE_FORCE_NULL
>> + *      if the type OID is invalid or unknown, return NULL instead of ???
>> + *      or such
>
> I think FORCE_NULL is a strange name for this flag (and its two
> siblings); I think something like FORMAT_TYPE_INVALID_AS_NULL is
> clearer.

Good idea.  I see the point.  Patches 0001 and 0002 are an easy cut.
For patch 0003, we could also argue if we'd want another flavor of
getObjectIdentity() (an "extended" version) which takes an extra
argument so as we reduce the size of the patch as there would be no
need to update all the existing callers (note: I still think no on
this one).

> I have still to review this comprehensively, but one thing I don't quite
> like is the shape of the new regression tests, which seem a bit too
> bulky ... why not use the style elsewhere in that file, with a large
> VALUES clause to supply input params for a single query? That would be
> a lot faster, too.

That makes sense.  Here is how I would write it then:
WITH objects (classid, objid, objsubid) AS (VALUES
    ('pg_class'::regclass, 0, 0), -- no relation
    [ ... ]
  )
SELECT ROW(pg_identify_object(objects.classid, objects.objid, objects.objsubid))
         AS ident,
       ROW(pg_identify_object_as_address(objects.classid, objects.objid, objects.objsubid))
         AS addr,
       pg_describe_object(objects.classid, objects.objid, objects.objsubid)
         AS descr,
FROM objects
ORDER BY objects.classid, objects.objid, objects.objsubid;
--
Michael

> On Tue, Sep 24, 2019 at 1:58 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> Please feel free to use the updated versions attached.  These can
> apply on top of HEAD at 30d1379.

Thanks for the update. Looking at v17 0003 I have one more question. In all the
places where we have to do systable_endscan, it followed by heap_close,
although in the rest of the file table_close is used. I guess this logic is not
heap specific and should also use table_close?

On Wed, Oct 16, 2019 at 01:04:57PM +0200, Dmitry Dolgov wrote:
> Thanks for the update. Looking at v17 0003 I have one more question. In all the
> places where we have to do systable_endscan, it followed by heap_close,
> although in the rest of the file table_close is used. I guess this logic is not
> heap specific and should also use table_close?

We need to use table_close as we cannot assume that the relation will
always be opened for heap.  This patch set is around for so long, so
that was a rotten part still able to compile as we have a macro to
cover the gap.  Thanks for noticing that.  I am sending an updated
patch set in a but, Alvaro had more comments.
--
Michael

On Thu, Sep 26, 2019 at 03:52:03PM +0900, Michael Paquier wrote:
> On Wed, Sep 25, 2019 at 09:21:03AM -0300, Alvaro Herrera wrote:
>> On 2019-Sep-24, Michael Paquier wrote:
>>> + * - FORMAT_TYPE_FORCE_NULL
>>> + *      if the type OID is invalid or unknown, return NULL instead of ???
>>> + *      or such
>>
>> I think FORCE_NULL is a strange name for this flag (and its two
>> siblings); I think something like FORMAT_TYPE_INVALID_AS_NULL is
>> clearer.
>
> Good idea.  I see the point.

Got to think more on this one, and your idea is better.  So changed
this way.

>> I have still to review this comprehensively, but one thing I don't quite
>> like is the shape of the new regression tests, which seem a bit too
>> bulky ... why not use the style elsewhere in that file, with a large
>> VALUES clause to supply input params for a single query? That would be
>> a lot faster, too.
>
> That makes sense.  Here is how I would write it then:
> WITH objects (classid, objid, objsubid) AS (VALUES
>     ('pg_class'::regclass, 0, 0), -- no relation
>     [ ... ]
>   )
> SELECT ROW(pg_identify_object(objects.classid, objects.objid, objects.objsubid))
>          AS ident,
>        ROW(pg_identify_object_as_address(objects.classid, objects.objid, objects.objsubid))
>          AS addr,
>        pg_describe_object(objects.classid, objects.objid, objects.objsubid)
>          AS descr,
> FROM objects
> ORDER BY objects.classid, objects.objid, objects.objsubid;

That's actually cleaner, so changed as well.  Attached is an updated
patch set with the heap_close() calls removed as per the recent report
from Dmitry.
--
Michael

> On 17 Oct 2019, at 03:37, Michael Paquier <michael@paquier.xyz> wrote:

> Attached is an updated
> patch set with the heap_close() calls removed as per the recent report
> from Dmitry.

Taking a look at this to see if we can achieve closure on this long-running
patchset.  The goal of the patch is IMO a clear win for usability.

The patchset applies with a bit of fuzzing and offsetting, it has tests (which
pass) as well as the relevant documentation changes.  I agree with the previous
reviewers that the new shape of the test is better, so definitely +1 on that
change.  There are a lot of mechanic changes in this set, but AFAICT they cover
all the bases.

Since the patch has been through a lot of review already there isn't a lot to
say, only a few very minor things that stood out:

  * This exports the useful functionality of regoperatorout for use
  * in other backend modules.  The result is a palloc'd string.
format_operator_extended has this comment, but the code can now also return
NULL and not just a palloc'd string.

+                   if (!missing_ok)
+                   {
+                       elog(ERROR, "could not find tuple for cast %u",
+                            object->objectId);
+                   }
The other (!missing_ok) blocks in this patch are not encapsulating the elog()
with braces.

I'm switching this to ready for committer,

cheers ./daniel

On Thu, Mar 19, 2020 at 08:48:58AM +0100, Daniel Gustafsson wrote:
> Taking a look at this to see if we can achieve closure on this long-running
> patchset.  The goal of the patch is IMO a clear win for usability.
>
> The patchset applies with a bit of fuzzing and offsetting, it has tests (which
> pass) as well as the relevant documentation changes.  I agree with the previous
> reviewers that the new shape of the test is better, so definitely +1 on that
> change.  There are a lot of mechanic changes in this set, but AFAICT they cover
> all the bases.

Thanks.  I hope it is helpful.

> Since the patch has been through a lot of review already there isn't a lot to
> say, only a few very minor things that stood out:
>
>   * This exports the useful functionality of regoperatorout for use
>   * in other backend modules.  The result is a palloc'd string.
> format_operator_extended has this comment, but the code can now also return
> NULL and not just a palloc'd string.
>
> +                   if (!missing_ok)
> +                   {
> +                       elog(ERROR, "could not find tuple for cast %u",
> +                            object->objectId);
> +                   }
> The other (!missing_ok) blocks in this patch are not encapsulating the elog()
> with braces.
>
> I'm switching this to ready for committer,

The new FORMAT_TYPE_* flag still makes sense to me while reading this
code once again, as well as the extensibility of the new APIs for
operators and procedures.  One doubt I still have is if we should
really change the signature of getObjectDescription() to include the
new missing_ok argument or if a new function should be introduced.
I'd rather keep only one function as, even if this is called in many
places in the backend, I cannot track down an extension using it, but
I won't go against Alvaro's will either if he thinks something
different as this is his original design and commit as of f8348ea3.
--
Michael

On Thu, Mar 19, 2020 at 10:30:11PM +0900, Michael Paquier wrote:
> The new FORMAT_TYPE_* flag still makes sense to me while reading this
> code once again, as well as the extensibility of the new APIs for
> operators and procedures.  One doubt I still have is if we should
> really change the signature of getObjectDescription() to include the
> new missing_ok argument or if a new function should be introduced.
> I'd rather keep only one function as, even if this is called in many
> places in the backend, I cannot track down an extension using it, but
> I won't go against Alvaro's will either if he thinks something
> different as this is his original design and commit as of f8348ea3.

Attached is an updated patch set, because of conflicts in the docs.
Daniel, you are still registered as a reviewer of this patch, and it
is marked as ready for committer?  Do you have more comments?  Would
anybody have objections if I move on with 0001 and 0002 that extend
the APIs to get the procedure, operator and type names?
--
Michael

> On 2 Jul 2020, at 07:35, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Mar 19, 2020 at 10:30:11PM +0900, Michael Paquier wrote:
>> The new FORMAT_TYPE_* flag still makes sense to me while reading this
>> code once again, as well as the extensibility of the new APIs for
>> operators and procedures.  One doubt I still have is if we should
>> really change the signature of getObjectDescription() to include the
>> new missing_ok argument or if a new function should be introduced.
>> I'd rather keep only one function as, even if this is called in many
>> places in the backend, I cannot track down an extension using it, but
>> I won't go against Alvaro's will either if he thinks something
>> different as this is his original design and commit as of f8348ea3.
>
> Attached is an updated patch set, because of conflicts in the docs.
> Daniel, you are still registered as a reviewer of this patch, and it
> is marked as ready for committer?  Do you have more comments?  Would
> anybody have objections if I move on with 0001 and 0002 that extend
> the APIs to get the procedure, operator and type names?

No objections from me after skimming over the updated patchset.  I still
maintain that the format_operator_extended comment should be amended to include
that it can return NULL and not alwatys palloced string, but that's it.

cheers ./daniel

On 2020-Jul-02, Michael Paquier wrote:

> Attached is an updated patch set, because of conflicts in the docs.
> Daniel, you are still registered as a reviewer of this patch, and it
> is marked as ready for committer?  Do you have more comments?  Would
> anybody have objections if I move on with 0001 and 0002 that extend
> the APIs to get the procedure, operator and type names?

0001 and 0002 look good to me.

I think 0003 could be a little more careful about indentation; some long
lines are going to result after pgindent that might be better to handle
in a different way before commit, e.g., here

> +            {
> +                char *proname = format_procedure_extended(object->objectId,
> +                        FORMAT_PROC_FORCE_QUALIFY | FORMAT_PROC_INVALID_AS_NULL);

I don't have any substantive comments.

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

On Fri, Jul 03, 2020 at 12:34:39PM +0200, Daniel Gustafsson wrote:
> No objections from me after skimming over the updated patchset.  I still
> maintain that the format_operator_extended comment should be amended to include
> that it can return NULL and not alwatys palloced string, but that's it.

Okay, 0001 and 0002 are now committed.  The same comment applies to
the procedure part, and I have noticed three more inconsistencies in
0002, the comments of format_procedure_extended had better match its
operator sibling, the new declarations in regproc.h still used
type_oid for the first argument, and the description of the flags
still referred to a type name for both routines.  I am looking at
0003 with fresh eyes now, and I'll try to send a rebased version
shortly.
--
Michael

On Fri, Jul 03, 2020 at 11:04:17AM -0400, Alvaro Herrera wrote:
> 0001 and 0002 look good to me.

Thanks for the review.

> I think 0003 could be a little more careful about indentation; some long
> lines are going to result after pgindent that might be better to handle
> in a different way before commit, e.g., here
>
>> +    {
>> +        char *proname = format_procedure_extended(object->objectId,
>> +            FORMAT_PROC_FORCE_QUALIFY | FORMAT_PROC_INVALID_AS_NULL);

Yes, I was looking at that for a couple of hours and pgindent made
that a bit weird.  So I have changed the code to just use a separate
variable.  That makes things a bit cleaner.

While refreshing my mind with this code, I got surprised with the
choice of "routine" in getProcedureTypeDescription() when we need a
default object type name for an object not found, so I have switched
that to "procedure" to be more consistent.

I have also spent some time analyzing the coverage of the patch, and
did not find any obvious holes or any remaining missing_ok paths not
covered.  Some comments were also a bit weird after re-reading them,
so I tweaked a couple of places.

Attached is for now a rebased patch.  If there are any comments,
please feel free.  Daniel, Alvaro, does that look fine for you?  I am
letting this stuff aside for a couple of days for the moment.
--
Michael

On 2020-Jul-06, Michael Paquier wrote:

> While refreshing my mind with this code, I got surprised with the
> choice of "routine" in getProcedureTypeDescription() when we need a
> default object type name for an object not found, so I have switched
> that to "procedure" to be more consistent.

I think "routine" was more correct.  We introduced that terminology to
encompass both functions and procedures, back when real procedures were
added.  See the definitions in the glossary for example.

> I have also spent some time analyzing the coverage of the patch, and
> did not find any obvious holes or any remaining missing_ok paths not
> covered.  Some comments were also a bit weird after re-reading them,
> so I tweaked a couple of places.

Cool.

> Attached is for now a rebased patch.  If there are any comments,
> please feel free.  Daniel, Alvaro, does that look fine for you?  I am
> letting this stuff aside for a couple of days for the moment.

I'm not sure that I'll have time to review this in the next couple of
days, but it seemed sufficiently good when I last looked.

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

On Mon, Jul 06, 2020 at 10:56:53AM -0400, Alvaro Herrera wrote:
> I think "routine" was more correct.  We introduced that terminology to
> encompass both functions and procedures, back when real procedures were
> added.  See the definitions in the glossary for example.

Okay, fine by me, while we have getProcedureTypeDescription() working
on pg_proc entries ;)

> I'm not sure that I'll have time to review this in the next couple of
> days, but it seemed sufficiently good when I last looked.

Thanks.  I'll try to come back to it next week or the one after for
another round of self-review, and I'll see what to do at this point.
This has been around for three years, it can still wait a bit.
--
Michael

> On 6 Jul 2020, at 09:45, Michael Paquier <michael@paquier.xyz> wrote:

> Attached is for now a rebased patch.  If there are any comments,
> please feel free.  Daniel, Alvaro, does that look fine for you?  I am
> letting this stuff aside for a couple of days for the moment.

Reading through I have no comments or objections, LGTM.  I didn't try to apply
and run tests, but the CFBot is happy about it so I doubt I would find anything
different there.

cheers ./daniel

On 2020-Jul-07, Michael Paquier wrote:

> On Mon, Jul 06, 2020 at 10:56:53AM -0400, Alvaro Herrera wrote:
> > I think "routine" was more correct.  We introduced that terminology to
> > encompass both functions and procedures, back when real procedures were
> > added.  See the definitions in the glossary for example.
> 
> Okay, fine by me, while we have getProcedureTypeDescription() working
> on pg_proc entries ;)

That's a holdover from old times, when we thought functions were
procedures.  That's no longer the case.




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

On Tue, Jul 07, 2020 at 11:08:30AM -0400, Alvaro Herrera wrote:
g> That's a holdover from old times, when we thought functions were
> procedures.  That's no longer the case.

Thanks, so "routine" it is.  I have done an extra round of review of
this patch, and noticed two things:
- In getObjectDescription(), we were doing a call to get_attname() for
nothing with OCLASS_CLASS with an attribute.
- The regression test output has been changed to use \a\t to make
future diffs more readable if we add an object type that increases the
column size.

And applied the change.  Thanks to everybody who took the time to look
at this code and comment about it.  It took actually less than 3 years
for this threadto conclude, as it began on the 19th of July, 2017.
--
Michael