Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Daniel Gustafsson]

> On 6 Jul 2020, at 09:45, Michael Paquier  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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 3ca2841d38880eb0f40427e9aa447b5ae3a2e66c Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Mon, 6 Jul 2020 16:44:31 +0900
Subject: [PATCH v20] Eliminate user-visible cache lookup errors for objaddr
 SQL functions

When using the following functions, users could see various types of
errors like "cache lookup failed for OID XXX":
* pg_describe_object
* pg_identify_object_as_address
* pg_identify_object
All the lower set of APIs managing object addresses for all types of
the system are made smarter by gaining a missing_ok argument that allows
any caller to control if he is willing to have an actual error or if
he wants to control error at its level by having empty objects if they
are undefined.

Regression tests added in this commit failed with such errors before
being patched.
---
 src/include/catalog/objectaddress.h  |   12 +-
 src/include/utils/regproc.h  |5 +-
 src/backend/catalog/dependency.c |   30 +-
 src/backend/catalog/objectaddress.c  | 1003 +-
 src/backend/catalog/pg_depend.c  |4 +-
 src/backend/catalog/pg_shdepend.c|8 +-
 src/backend/commands/event_trigger.c |9 +-
 src/backend/commands/extension.c |6 +-
 src/backend/commands/tablecmds.c |   12 +-
 src/backend/utils/adt/regproc.c  |   20 +-
 src/test/regress/expected/object_address.out |   98 ++
 src/test/regress/sql/object_address.sql  |   56 +
 doc/src/sgml/func.sgml   |7 +-
 contrib/sepgsql/database.c   |6 +-
 contrib/sepgsql/dml.c|4 +-
 contrib/sepgsql/label.c  |4 +-
 contrib/sepgsql/proc.c   |   14 +-
 contrib/sepgsql/relation.c   |   20 +-
 contrib/sepgsql/schema.c |6 +-
 19 files changed, 996 insertions(+), 328 deletions(-)

diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h
index 144715d4f4..b876617c9d 100644
--- a/src/include/catalog/objectaddress.h
+++ b/src/include/catalog/objectaddress.h
@@ -70,14 +70,18 @@ extern bool get_object_namensp_unique(Oid class_id);
 extern HeapTuple get_catalog_object_by_oid(Relation catalog,
 		   AttrNumber oidcol, Oid objectId);
 
-extern char *getObjectDescription(const ObjectAddress *object);
+extern char *getObjectDescription(const ObjectAddress *object,
+  bool missing_ok);
 extern char *getObjectDescriptionOids(Oid classid, Oid objid);
 
 extern int	read_objtype_from_string(const char *objtype);
-extern char *getObjectTypeDescription(const ObjectAddress *object);
-extern char *getObjectIdentity(const ObjectAddress *address);
+extern char *getObjectTypeDescription(const ObjectAddress *object,
+	  bool missing_ok);
+extern char *getObjectIdentity(const ObjectAddress *address,
+			   bool missing_ok);
 extern char *getObjectIdentityParts(const ObjectAddress *address,
-	List **objname, List **objargs);
+	List **objname, List **objargs,
+	bool missing_ok);
 extern struct ArrayType *strlist_to_textarray(List *list);
 
 extern ObjectType get_relkind_objtype(char relkind);
diff --git a/src/include/utils/regproc.h b/src/include/utils/regproc.h
index 145452a5ad..330417e888 100644
--- a/src/include/utils/regproc.h
+++ b/src/include/utils/regproc.h
@@ -29,10 +29,11 @@ extern List *stringToQualifiedNameList(const char *string);
 extern char *format_procedure(Oid procedure_oid);
 extern char *format_procedure_qualified(Oid procedure_oid);
 extern void format_procedure_parts(Oid 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Daniel Gustafsson]

> On 2 Jul 2020, at 07:35, Michael Paquier  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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From f0c8ffafcbcd0a43ff9729321cd68dd592a54c5c Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 17 Oct 2019 10:25:24 +0900
Subject: [PATCH v19 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_INVALID_AS_NULL which
returns a NULL result instead of "??? or "-" which does not generate an
error. They will be used for future patches to improve the SQL interface
for object addresses.
---
 src/include/utils/builtins.h|  1 +
 src/backend/utils/adt/format_type.c | 22 +-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index f8595642da..3ca5e938f8 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -113,6 +113,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_INVALID_AS_NULL	0x08	/* NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index df0bdeb610..f2816e4f37 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -96,13 +96,16 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_INVALID_AS_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
  * Note that TYPEMOD_GIVEN is not interchangeable with "typemod == -1";
  * see the comments above for format_type().
  *
- * Returns a palloc'd string.
+ * Returns a palloc'd string, or NULL.
  */
 char *
 format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
@@ -114,13 +117,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	if (type_oid == InvalidOid && (flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
-		return pstrdup("-");
+	if (type_oid == InvalidOid)
+	{
+		if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			return pstrdup("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -144,7 +154,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
-- 
2.27.0

From fd33019e5e0c41aa8b64e7ed429fcf468695ce99 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 17 Oct 2019 10:25:39 +0900
Subject: [PATCH v19 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Daniel Gustafsson]

> On 17 Oct 2019, at 03:37, Michael Paquier  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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 3d42d9faf77bf6345694aaee8ce5e2317238d362 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 17 Oct 2019 10:25:24 +0900
Subject: [PATCH v18 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_INVALID_AS_NULL which
returns a NULL result instead of "??? or "-" which does not generate an
error. They will be used for future patches to improve the SQL interface
for object addresses.
---
 src/backend/utils/adt/format_type.c | 22 +-
 src/include/utils/builtins.h|  1 +
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 6ae2a31345..8696b11a9e 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -96,13 +96,16 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_INVALID_AS_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
  * Note that TYPEMOD_GIVEN is not interchangeable with "typemod == -1";
  * see the comments above for format_type().
  *
- * Returns a palloc'd string.
+ * Returns a palloc'd string, or NULL.
  */
 char *
 format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
@@ -114,13 +117,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	if (type_oid == InvalidOid && (flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
-		return pstrdup("-");
+	if (type_oid == InvalidOid)
+	{
+		if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			return pstrdup("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -143,7 +153,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_INVALID_AS_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 937ddb7ef0..44298d7410 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Dmitry Dolgov]

> On Tue, Sep 24, 2019 at 1:58 AM Michael Paquier  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?

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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
> > ''" 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Kyotaro Horiguchi]

At Tue, 24 Sep 2019 08:58:50 +0900, Michael Paquier  wrote 
in <20190923235850.ga2...@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
''" or something like.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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(, _("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
From dd4b4112eea12b4866aa2c6a33de57302b842c8a Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 24 Sep 2019 08:54:37 +0900
Subject: [PATCH v17 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_FORCE_NULL which returns
a NULL result instead of "??? or "-" which does not generate an error.
They will be used for future patches to improve the SQL interface for
object addresses.
---
 src/backend/utils/adt/format_type.c | 22 +-
 src/include/utils/builtins.h|  1 +
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 6ae2a31345..076d2ca3f3 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -96,13 +96,16 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_FORCE_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
  * Note that TYPEMOD_GIVEN is not interchangeable with "typemod == -1";
  * see the comments above for format_type().
  *
- * Returns a palloc'd string.
+ * Returns a palloc'd string, or NULL.
  */
 char *
 format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
@@ -114,13 +117,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	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("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -143,7 +153,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 937ddb7ef0..78071b7755 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_FORCE_NULL		0x08	/* force NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
-- 
2.23.0

From de4bae9cbdfb9d88c77221506f167dd4d84ebd57 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 24 Sep 2019 08:54:52 +0900
Subject: [PATCH v17 2/3] Refactor 

Re: Cache lookup errors with functions manipulation object addresses

[Dmitry Dolgov]

> On Tue, Jul 2, 2019 at 9:28 AM Michael Paquier  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(, _("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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 76ef961e02e0276af5516bdce6e0e543526db5b2 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 2 Jul 2019 16:07:06 +0900
Subject: [PATCH v16 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_FORCE_NULL which returns
a NULL result instead of "??? or "-" which does not generate an error.
They will be used for future patches to improve the SQL interface for
object addresses.
---
 src/backend/utils/adt/format_type.c | 20 
 src/include/utils/builtins.h|  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 6ae2a31345..8ee0e10c29 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -96,6 +96,9 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_FORCE_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
@@ -114,13 +117,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	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("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -143,7 +153,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index a3cd7d26fa..a1e418cee7 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_FORCE_NULL		0x08	/* force NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
-- 
2.20.1

From ad8f7f57f91b243926740d5a224357f54295ee34 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 2 Jul 2019 16:07:29 +0900
Subject: [PATCH v16 2/3] Refactor format procedure and operator APIs to be
 more modular

This introduce a new set of extended routines for procedure and operator
lookups, with a flags bitmask argument that can modify the default
behavior:
- Force schema qualification
- Force NULL as result instead of a numeric OID for an undefined
object.
---
 src/backend/utils/adt/regproc.c | 61 +++--
 src/include/utils/regproc.h | 10 ++
 2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 17a7f6c9d8..8d6d73039b 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -40,8 +40,6 @@
 #include "utils/regproc.h"
 #include "utils/varlena.h"
 
-static char *format_operator_internal(Oid operator_oid, bool force_qualify);
-static char *format_procedure_internal(Oid procedure_oid, bool force_qualify);
 static void parseNameAndArgTypes(const char *string, bool allowNone,
  List **names, int *nargs, Oid *argtypes);
 
@@ -322,24 +320,27 @@ to_regprocedure(PG_FUNCTION_ARGS)
 char *
 format_procedure(Oid 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 5844b0ba07fdd8373a7ef15a1676178cae53c40d Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 12:21:24 +0900
Subject: [PATCH 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_FORCE_NULL which returns
a NULL result instead of "??? or "-" which does not generate an error.
They will be used for future patches to improve the SQL interface for
object addresses.
---
 src/backend/utils/adt/format_type.c | 20 
 src/include/utils/builtins.h|  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 6ae2a31345..8ee0e10c29 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -96,6 +96,9 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_FORCE_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
@@ -114,13 +117,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	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("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -143,7 +153,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 03d0ee2766..43e7ef471c 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_FORCE_NULL		0x08	/* force NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
-- 
2.20.1

From 2590b4ba96851f5ffea5f438e2e82383fdb74dae Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 13:13:28 +0900
Subject: [PATCH 2/3] Refactor format procedure and operator APIs to be more
 modular

This introduce a new set of extended routines for procedure and operator
lookups, with a flags bitmask argument that can modify the default
behavior:
- Force schema qualification
- Force NULL as result instead of a numeric OID for an undefined
object.
---
 src/backend/utils/adt/regproc.c | 61 +++--
 src/include/utils/regproc.h | 10 ++
 2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 09cf0e1abc..f85b47821a 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -40,8 +40,6 @@
 #include "utils/regproc.h"
 #include "utils/varlena.h"
 
-static char *format_operator_internal(Oid operator_oid, bool force_qualify);
-static char *format_procedure_internal(Oid procedure_oid, bool force_qualify);
 static void parseNameAndArgTypes(const char *string, bool allowNone,
 	 List **names, int *nargs, Oid *argtypes);
 
@@ -322,24 +320,27 @@ to_regprocedure(PG_FUNCTION_ARGS)
 char *
 format_procedure(Oid procedure_oid)
 {
-	return 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 6514d737cef100ea8146b1d6d10fa93e40c6ee9c Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 12:21:24 +0900
Subject: [PATCH 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_FORCE_NULL which returns
a NULL result instead of "??? or "-" which does not generate an error.
They will be used for future patches to improve the SQL interface for
object addresses.
---
 src/backend/utils/adt/format_type.c | 20 
 src/include/utils/builtins.h|  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index 84a1b3c72b..6fc8f55668 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -98,6 +98,9 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_FORCE_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
@@ -116,13 +119,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	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("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -145,7 +155,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 03d0ee2766..43e7ef471c 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_FORCE_NULL		0x08	/* force NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
-- 
2.20.1

From 3eaf6833c367549b9309b9df7ef7d56f8e596cc3 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 13:13:28 +0900
Subject: [PATCH 2/3] Refactor format procedure and operator APIs to be more
 modular

This introduce a new set of extended routines for procedure and operator
lookups, with a flags bitmask argument that can modify the default
behavior:
- Force schema qualification
- Force NULL as result instead of a numeric OID for an undefined
object.
---
 src/backend/utils/adt/regproc.c | 61 +++--
 src/include/utils/regproc.h | 10 ++
 2 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/src/backend/utils/adt/regproc.c b/src/backend/utils/adt/regproc.c
index 09cf0e1abc..f85b47821a 100644
--- a/src/backend/utils/adt/regproc.c
+++ b/src/backend/utils/adt/regproc.c
@@ -40,8 +40,6 @@
 #include "utils/regproc.h"
 #include "utils/varlena.h"
 
-static char *format_operator_internal(Oid operator_oid, bool force_qualify);
-static char *format_procedure_internal(Oid procedure_oid, bool force_qualify);
 static void parseNameAndArgTypes(const char *string, bool allowNone,
 	 List **names, int *nargs, Oid *argtypes);
 
@@ -322,24 +320,27 @@ 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

Á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
From 5f62f834ffb5f310e6469f6e7496f898cefb26ac Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 12:21:24 +0900
Subject: [PATCH 1/3] Add flag to format_type_extended to enforce NULL-ness

If a type scanned is undefined, type format routines have two behaviors
depending on if FORMAT_TYPE_ALLOW_INVALID is defined by the caller:
- Generate an error
- Return undefined type name "???" or "-".

The current interface is unhelpful for callers willing to format
properly a type name, but still make sure that the type is defined as
there could be types matching the undefined strings.  In order to
counter that, add a new flag called FORMAT_TYPE_FORCE_NULL which returns
a NULL result instead of "??? or "-" which does not generate an error.
They will be used for future patches to improve the SQL interface for
object addresses.
---
 src/backend/utils/adt/format_type.c | 20 
 src/include/utils/builtins.h|  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/format_type.c b/src/backend/utils/adt/format_type.c
index f1f0ba3e0b..c41271ba4c 100644
--- a/src/backend/utils/adt/format_type.c
+++ b/src/backend/utils/adt/format_type.c
@@ -98,6 +98,9 @@ format_type(PG_FUNCTION_ARGS)
  * - FORMAT_TYPE_ALLOW_INVALID
  *			if the type OID is invalid or unknown, return ??? or such instead
  *			of failing
+ * - FORMAT_TYPE_FORCE_NULL
+ *			if the type OID is invalid or unknown, return NULL instead of ???
+ *			or such
  * - FORMAT_TYPE_FORCE_QUALIFY
  *			always schema-qualify type names, regardless of search_path
  *
@@ -116,13 +119,20 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 	char	   *buf;
 	bool		with_typemod;
 
-	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("-");
+	}
 
 	tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(type_oid));
 	if (!HeapTupleIsValid(tuple))
 	{
-		if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+		if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+			return NULL;
+		else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 			return pstrdup("???");
 		else
 			elog(ERROR, "cache lookup failed for type %u", type_oid);
@@ -145,7 +155,9 @@ format_type_extended(Oid type_oid, int32 typemod, bits16 flags)
 		tuple = SearchSysCache1(TYPEOID, ObjectIdGetDatum(array_base_type));
 		if (!HeapTupleIsValid(tuple))
 		{
-			if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
+			if ((flags & FORMAT_TYPE_FORCE_NULL) != 0)
+return NULL;
+			else if ((flags & FORMAT_TYPE_ALLOW_INVALID) != 0)
 return pstrdup("???[]");
 			else
 elog(ERROR, "cache lookup failed for type %u", type_oid);
diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h
index 61785a2433..8a17164bb7 100644
--- a/src/include/utils/builtins.h
+++ b/src/include/utils/builtins.h
@@ -109,6 +109,7 @@ extern Datum numeric_float8_no_overflow(PG_FUNCTION_ARGS);
 #define FORMAT_TYPE_TYPEMOD_GIVEN	0x01	/* typemod defined by caller */
 #define FORMAT_TYPE_ALLOW_INVALID	0x02	/* allow invalid types */
 #define FORMAT_TYPE_FORCE_QUALIFY	0x04	/* force qualification of type */
+#define FORMAT_TYPE_FORCE_NULL		0x08	/* force NULL if undefined */
 extern char *format_type_extended(Oid type_oid, int32 typemod, bits16 flags);
 
 extern char *format_type_be(Oid type_oid);
-- 
2.20.1

From a35f6d0f42085cd4db15d9c63a23847d0cdaca96 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 25 Dec 2018 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From 7d832462d64f8d8df417682cff234876ac154b41 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 13 Dec 2018 14:29:19 +0900
Subject: [PATCH 1/2] Introduce new extended routines for FDW and foreign
 server lookups
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument to handle a set of flags.  The only
value which can be used now is to indicate if a missing object should
result in an error or not, and are designed to be extensible on need.
Those new routines are added into the existing set of user-visible
things and documented in consequence.

Author: Michael Paquier
Reviewed-by: Álvaro Herrera
Discussion: https://postgr.es/m/CAB7nPqSZxrSmdHK-rny7z8mi=EAFXJ5J-0RbzDw6aus=wb5...@mail.gmail.com
---
 doc/src/sgml/fdwhandler.sgml  | 34 +
 src/backend/foreign/foreign.c | 36 +--
 src/include/foreign/foreign.h | 10 ++
 3 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 4ce88dd77c..452b776b9e 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1408,6 +1408,23 @@ ReparameterizeForeignPathByChild(PlannerInfo *root, List *fdw_private,
 
 
 ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bits16 flags);
+
+
+ This function returns a ForeignDataWrapper
+ object for the foreign-data wrapper with the given OID.  A
+ ForeignDataWrapper object contains properties
+ of the 

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From f4dd9741833e4499c1d7698dc869e2f8b9bfb5ce Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 18 Sep 2018 14:17:09 +0900
Subject: [PATCH 1/2] Extend lookup routines for FDW and foreign server with
 NULL handling

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument to pass flags.  The only value which
can be used now is to indicate if it a missing object should result in
an error or not.  Those new routines are added into the existing set and
documented in consequence.
---
 doc/src/sgml/fdwhandler.sgml  | 34 +
 src/backend/foreign/foreign.c | 36 +--
 src/include/foreign/foreign.h | 16 
 3 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 4ce88dd77c..6fe6122568 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1408,6 +1408,23 @@ ReparameterizeForeignPathByChild(PlannerInfo *root, List *fdw_private,
 
 
 ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, uint32 flags);
+
+
+ This function returns a ForeignDataWrapper
+ object for the foreign-data wrapper with the given OID.  A
+ ForeignDataWrapper object contains properties
+ of the FDW (see foreign/foreign.h for details).
+ flags is a bitwise-or'd bit mask indicating
+ an extra set of options.  It can take the value
+ FDW_MISSING_OK, in which case a NULL
+ result is returned to the caller instead of an error for an undefined
+ object.
+
+
+
+
+ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid);
 
 
@@ -1420,6 +1437,23 @@ GetForeignDataWrapper(Oid fdwid);
 
 
 ForeignServer *
+GetForeignServerExtended(Oid serverid, uint32 flags);
+
+
+ This function returns a ForeignServer object
+ for the foreign server with the given OID.  A
+ ForeignServer object contains properties
+ of the server (see foreign/foreign.h for details).
+ flags is a bitwise-or'd bit mask indicating
+ an extra set of options.  It can take the value
+ FSV_MISSING_OK, in which case a NULL
+ result is returned to the caller instead of an error for an undefined
+ object.
+
+
+
+
+ForeignServer *
 GetForeignServer(Oid serverid);
 
 
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index eac78a5d31..88726d9d70 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -33,6 +33,18 @@
  */
 ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid)
+{
+	return GetForeignDataWrapperExtended(fdwid, 0);
+}
+
+
+/*
+ * GetForeignDataWrapperExtended -	look up the foreign-data wrapper
+ * by OID. If flags uses FDW_MISSING_OK, return NULL if the object cannot
+ * be found instead of raising an error.
+ */
+ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, uint32 flags)
 {
 	Form_pg_foreign_data_wrapper fdwform;
 	ForeignDataWrapper *fdw;
@@ -43,7 

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From fba6464f1555ec05029a58e2bf378ef83ce73172 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sun, 1 Jul 2018 23:26:10 +0900
Subject: [PATCH 1/3] Extend lookup routines for FDW and foreign server with
 NULL handling

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument able to control if an error or a
NULL object is returned to the caller in the event of an undefined
object. This is added in a set of new routines to not impact
unnecessrily any FPW plugins.
---
 doc/src/sgml/fdwhandler.sgml  | 30 +
 src/backend/foreign/foreign.c | 36 +--
 src/include/foreign/foreign.h |  4 
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 4ce88dd77c..a68e264261 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1408,6 +1408,21 @@ ReparameterizeForeignPathByChild(PlannerInfo *root, List *fdw_private,
 
 
 ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok);
+
+
+ This function returns a ForeignDataWrapper
+ object for the foreign-data wrapper with the given OID.  A
+ ForeignDataWrapper object contains properties
+ of the FDW (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ FDW.
+
+
+
+
+ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid);
 
 
@@ -1420,6 +1435,21 @@ GetForeignDataWrapper(Oid fdwid);
 
 
 ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok);
+
+
+ This function returns a ForeignServer object
+ for the foreign server with the given OID.  A
+ ForeignServer object contains properties
+ of the server (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ foreign server.
+
+
+
+
+ForeignServer *
 GetForeignServer(Oid serverid);
 
 
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index eac78a5d31..01b5175e71 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -33,6 +33,18 @@
  */
 ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid)
+{
+	return GetForeignDataWrapperExtended(fdwid, false);
+}
+
+
+/*
+ * GetForeignDataWrapperExtended -	look up the foreign-data wrapper
+ * by OID. If missing_ok is true, return NULL if the object cannot be
+ * found instead of raising an error.
+ */
+ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok)
 {
 	Form_pg_foreign_data_wrapper fdwform;
 	ForeignDataWrapper *fdw;
@@ -43,7 +55,11 @@ GetForeignDataWrapper(Oid fdwid)
 	tp = SearchSysCache1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+		return NULL;
+	}
 
 	fdwform = (Form_pg_foreign_data_wrapper) GETSTRUCT(tp);
 
@@ -91,6 +107,18 @@ GetForeignDataWrapperByName(const char *fdwname, bool missing_ok)
  */
 ForeignServer *
 GetForeignServer(Oid serverid)
+{
+	return GetForeignServerExtended(serverid, false);
+}
+
+
+/*
+ * GetForeignServerExtended - look up the foreign server definition. If
+ * missing_ok is true, return NULL if the object cannot be found instead
+ * of raising an error.
+ */
+ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok)
 {
 	Form_pg_foreign_server serverform;
 	ForeignServer *server;
@@ -101,7 +129,11 @@ GetForeignServer(Oid serverid)
 	tp = SearchSysCache1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+		return NULL;
+	}
 
 	serverform = (Form_pg_foreign_server) GETSTRUCT(tp);
 
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 3ca12e64d2..5cc89e967c 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -70,9 +70,13 @@ typedef struct ForeignTable
 
 
 extern ForeignServer *GetForeignServer(Oid serverid);
+extern 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Andrew Dunstan]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From ecd826a82b43011a831dc0b52d0b2a2d5d09fa9a Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Sun, 1 Jul 2018 23:26:10 +0900
Subject: [PATCH 1/3] Extend lookup routines for FDW and foreign server with
 NULL handling

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument able to control if an error or a
NULL object is returned to the caller in the event of an undefined
object. This is added in a set of new routines to not impact
unnecessrily any FPW plugins.
---
 doc/src/sgml/fdwhandler.sgml  | 30 +
 src/backend/foreign/foreign.c | 36 +--
 src/include/foreign/foreign.h |  4 
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 7b758bdf09..bfc538f249 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1404,6 +1404,21 @@ ReparameterizeForeignPathByChild(PlannerInfo *root, List *fdw_private,
 
 
 ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok);
+
+
+ This function returns a ForeignDataWrapper
+ object for the foreign-data wrapper with the given OID.  A
+ ForeignDataWrapper object contains properties
+ of the FDW (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ FDW.
+
+
+
+
+ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid);
 
 
@@ -1416,6 +1431,21 @@ GetForeignDataWrapper(Oid fdwid);
 
 
 ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok);
+
+
+ This function returns a ForeignServer object
+ for the foreign server with the given OID.  A
+ ForeignServer object contains properties
+ of the server (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ foreign server.
+
+
+
+
+ForeignServer *
 GetForeignServer(Oid serverid);
 
 
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index eac78a5d31..01b5175e71 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -33,6 +33,18 @@
  */
 ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid)
+{
+	return GetForeignDataWrapperExtended(fdwid, false);
+}
+
+
+/*
+ * GetForeignDataWrapperExtended -	look up the foreign-data wrapper
+ * by OID. If missing_ok is true, return NULL if the object cannot be
+ * found instead of raising an error.
+ */
+ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok)
 {
 	Form_pg_foreign_data_wrapper fdwform;
 	ForeignDataWrapper *fdw;
@@ -43,7 +55,11 @@ GetForeignDataWrapper(Oid fdwid)
 	tp = SearchSysCache1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+		return NULL;
+	}
 
 	fdwform = (Form_pg_foreign_data_wrapper) GETSTRUCT(tp);
 
@@ -91,6 +107,18 @@ GetForeignDataWrapperByName(const char *fdwname, bool missing_ok)
  */
 ForeignServer *
 GetForeignServer(Oid serverid)
+{
+	return GetForeignServerExtended(serverid, false);
+}
+
+
+/*
+ * GetForeignServerExtended - look up the foreign server definition. If
+ * missing_ok is true, return NULL if the object cannot be found instead
+ * of raising an error.
+ */
+ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok)
 {
 	Form_pg_foreign_server serverform;
 	ForeignServer *server;
@@ -101,7 +129,11 @@ GetForeignServer(Oid serverid)
 	tp = SearchSysCache1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+		return NULL;
+	}
 
 	serverform = (Form_pg_foreign_server) GETSTRUCT(tp);
 
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 3ca12e64d2..5cc89e967c 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -70,9 +70,13 @@ typedef struct ForeignTable
 
 
 

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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
From be6f8c7b986ea283e1f346b18b1c3116d1956772 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 13 Feb 2018 11:12:44 +0900
Subject: [PATCH 1/3] Extend lookup routines for FDW and foreign server with
 NULL handling

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument able to control if an error or a
NULL object is returned to the caller in the event of an undefined
object. This is added in a set of new routines to not impact
unnecessrily any FPW plugins.
---
 doc/src/sgml/fdwhandler.sgml  | 30 +
 src/backend/foreign/foreign.c | 36 +--
 src/include/foreign/foreign.h |  4 
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 7b758bdf09..bfc538f249 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1404,6 +1404,21 @@ ReparameterizeForeignPathByChild(PlannerInfo *root, List *fdw_private,
 
 
 ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok);
+
+
+ This function returns a ForeignDataWrapper
+ object for the foreign-data wrapper with the given OID.  A
+ ForeignDataWrapper object contains properties
+ of the FDW (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ FDW.
+
+
+
+
+ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid);
 
 
@@ -1416,6 +1431,21 @@ GetForeignDataWrapper(Oid fdwid);
 
 
 ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok);
+
+
+ This function returns a ForeignServer object
+ for the foreign server with the given OID.  A
+ ForeignServer object contains properties
+ of the server (see foreign/foreign.h for details).
+ If missing_ok is true, a NULL
+ result is returned to the caller instead of an error for an undefined
+ foreign server.
+
+
+
+
+ForeignServer *
 GetForeignServer(Oid serverid);
 
 
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index eac78a5d31..01b5175e71 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -33,6 +33,18 @@
  */
 ForeignDataWrapper *
 GetForeignDataWrapper(Oid fdwid)
+{
+	return GetForeignDataWrapperExtended(fdwid, false);
+}
+
+
+/*
+ * GetForeignDataWrapperExtended -	look up the foreign-data wrapper
+ * by OID. If missing_ok is true, return NULL if the object cannot be
+ * found instead of raising an error.
+ */
+ForeignDataWrapper *
+GetForeignDataWrapperExtended(Oid fdwid, bool missing_ok)
 {
 	Form_pg_foreign_data_wrapper fdwform;
 	ForeignDataWrapper *fdw;
@@ -43,7 +55,11 @@ GetForeignDataWrapper(Oid fdwid)
 	tp = SearchSysCache1(FOREIGNDATAWRAPPEROID, ObjectIdGetDatum(fdwid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign-data wrapper %u", fdwid);
+		return NULL;
+	}
 
 	fdwform = (Form_pg_foreign_data_wrapper) GETSTRUCT(tp);
 
@@ -91,6 +107,18 @@ GetForeignDataWrapperByName(const char *fdwname, bool missing_ok)
  */
 ForeignServer *
 GetForeignServer(Oid serverid)
+{
+	return GetForeignServerExtended(serverid, false);
+}
+
+
+/*
+ * GetForeignServerExtended - look up the foreign server definition. If
+ * missing_ok is true, return NULL if the object cannot be found instead
+ * of raising an error.
+ */
+ForeignServer *
+GetForeignServerExtended(Oid serverid, bool missing_ok)
 {
 	Form_pg_foreign_server serverform;
 	ForeignServer *server;
@@ -101,7 +129,11 @@ GetForeignServer(Oid serverid)
 	tp = SearchSysCache1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid));
 
 	if (!HeapTupleIsValid(tp))
-		elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+	{
+		if (!missing_ok)
+			elog(ERROR, "cache lookup failed for foreign server %u", serverid);
+		return NULL;
+	}
 
 	serverform = (Form_pg_foreign_server) GETSTRUCT(tp);
 
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 3ca12e64d2..5cc89e967c 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -70,9 +70,13 @@ typedef struct ForeignTable
 
 
 extern ForeignServer *GetForeignServer(Oid serverid);
+extern 

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

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

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

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


signature.asc
Description: PGP signature

Re: Cache lookup errors with functions manipulation object addresses

[Alvaro Herrera]

On 2018-Mar-06, Michael Paquier wrote:

> On Mon, Mar 05, 2018 at 12:57:38PM +, 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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Cache lookup errors with functions manipulation object addresses

[Michael Paquier]

On Mon, Mar 05, 2018 at 12:57:38PM +, 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
From 7fedf5f334acf0bddcf2f5672df368302ceb698b Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 13 Feb 2018 11:12:44 +0900
Subject: [PATCH 1/3] Extend lookup routines for FDW and foreign server with
 NULL handling

The cache lookup routines for foreign-data wrappers and foreign servers
are extended with an extra argument able to control if an error or a
NULL object is returned to the caller in the event of an undefined
object.
---
 contrib/dblink/dblink.c |  2 +-
 contrib/file_fdw/file_fdw.c |  4 ++--
 contrib/postgres_fdw/connection.c   |  4 ++--
 contrib/postgres_fdw/postgres_fdw.c |  8 
 doc/src/sgml/fdwhandler.sgml| 10 --
 src/backend/catalog/objectaddress.c | 12 ++--
 src/backend/commands/foreigncmds.c  | 14 --
 src/backend/commands/tablecmds.c|  8 
 src/backend/foreign/foreign.c   | 28 ++--
 src/include/foreign/foreign.h   |  4 ++--
 10 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index ae7e24ad08..a67febac6f 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -2784,7 +2784,7 @@ get_connect_string(const char *servername)
 		Oid			userid = GetUserId();
 
 		user_mapping = GetUserMapping(userid, serverid);
-		fdw = GetForeignDataWrapper(fdwid);
+		fdw = GetForeignDataWrapper(fdwid, false);
 
 		/* Check permissions, user must have usage on the server. */
 		aclresult = pg_foreign_server_aclcheck(serverid, userid, ACL_USAGE);
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index cf0a3629bc..d837f977e8 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -357,8 +357,8 @@ fileGetOptions(Oid foreigntableid,
 	 * Simplify?)
 	 */
 	table = GetForeignTable(foreigntableid);
-	server = GetForeignServer(table->serverid);
-	wrapper = GetForeignDataWrapper(server->fdwid);
+	server = GetForeignServer(table->serverid, false);
+	wrapper = GetForeignDataWrapper(server->fdwid, false);
 
 	options = NIL;
 	options = list_concat(options, wrapper->options);
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 00c926b983..3503595b7b 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -182,7 +182,7 @@ GetConnection(UserMapping *user, bool will_prep_stmt)
 	 */
 	if (entry->conn == NULL)
 	{
-		ForeignServer *server = GetForeignServer(user->serverid);
+		ForeignServer *server = GetForeignServer(user->serverid, false);
 
 		/* Reset all transient state fields, to be sure all are clean */
 		entry->xact_depth = 0;
@@ -1003,7 +1003,7 @@ pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry)
 	if (!HeapTupleIsValid(tup))
 		elog(ERROR, "cache lookup failed for user mapping %u", entry->key);
 	umform = (Form_pg_user_mapping) GETSTRUCT(tup);
-	server = GetForeignServer(umform->umserver);
+	server = GetForeignServer(umform->umserver, false);
 	ReleaseSysCache(tup);
 
 	ereport(ERROR,
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index e8a0d5482a..590469fe9e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -520,7 +520,7 @@ postgresGetForeignRelSize(PlannerInfo *root,
 
 	/* Look up foreign-table catalog info. */
 	fpinfo->table = GetForeignTable(foreigntableid);
-	fpinfo->server = GetForeignServer(fpinfo->table->serverid);
+	fpinfo->server = GetForeignServer(fpinfo->table->serverid, false);
 
 	/*
 	 * Extract user-settable option values.  Note that per-table setting of
@@ -2053,7 +2053,7 @@ postgresIsForeignRelUpdatable(Relation rel)
 	updatable = true;
 
 	table = GetForeignTable(RelationGetRelid(rel));
-	server = GetForeignServer(table->serverid);
+	server = GetForeignServer(table->serverid, false);
 
 	foreach(lc, server->options)
 	{
@@ -3998,7 +3998,7 @@ postgresAcquireSampleRowsFunc(Relation relation, int elevel,
 	 * owner, even if the ANALYZE was started by some other user.
 	 */
 	table = GetForeignTable(RelationGetRelid(relation));
-	server = GetForeignServer(table->serverid);
+	server = GetForeignServer(table->serverid, false);
 	user = GetUserMapping(relation->rd_rel->relowner, table->serverid);
 	conn = GetConnection(user, false);
 
@@ -4221,7 +4221,7 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
 	 * Get connection to the foreign server.  Connection manager will
 	 * establish new connection if necessary.
 	 */
-	server = 

Re: Cache lookup errors with functions manipulation object addresses

[Aleksander Alekseev]

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