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 <mich...@paquier.xyz> 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, <para> <programlisting> ForeignDataWrapper * +GetForeignDataWrapperExtended(Oid fdwid, bits16 flags); +</programlisting> + + This function returns a <structname>ForeignDataWrapper</structname> + object for the foreign-data wrapper with the given OID. A + <structname>ForeignDataWrapper</structname> object contains properties + of the FDW (see <filename>foreign/foreign.h</filename> for details). + <structfield>flags</structfield> is a bitwise-or'd bit mask indicating + an extra set of options. It can take the value + <literal>FDW_MISSING_OK</literal>, in which case a <literal>NULL</literal> + result is returned to the caller instead of an error for an undefined + object. + </para> + + <para> +<programlisting> +ForeignDataWrapper * GetForeignDataWrapper(Oid fdwid); </programlisting> @@ -1420,6 +1437,23 @@ GetForeignDataWrapper(Oid fdwid); <para> <programlisting> ForeignServer * +GetForeignServerExtended(Oid serverid, bits16 flags); +</programlisting> + + This function returns a <structname>ForeignServer</structname> object + for the foreign server with the given OID. A + <structname>ForeignServer</structname> object contains properties + of the server (see <filename>foreign/foreign.h</filename> for details). + <structfield>flags</structfield> is a bitwise-or'd bit mask indicating + an extra set of options. It can take the value + <literal>FSV_MISSING_OK</literal>, in which case a <literal>NULL</literal> + result is returned to the caller instead of an error for an undefined + object. + </para> + + <para> +<programlisting> +ForeignServer * GetForeignServer(Oid serverid); </programlisting> diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 989a58ad78..79661526a3 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, bits16 flags) { 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 ((flags & FDW_MISSING_OK) == 0) + 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, 0); +} + + +/* + * GetForeignServerExtended - look up the foreign server definition. If + * flags uses FSV_MISSING_OK, return NULL if the object cannot be found + * instead of raising an error. + */ +ForeignServer * +GetForeignServerExtended(Oid serverid, bits16 flags) { 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 ((flags & FSV_MISSING_OK) == 0) + 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..38c9784c8c 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -68,11 +68,21 @@ typedef struct ForeignTable List *options; /* ftoptions as DefElem list */ } ForeignTable; +/* Flags for GetForeignServerExtended */ +#define FSV_MISSING_OK 0x01 + +/* Flags for GetForeignDataWrapperExtended */ +#define FDW_MISSING_OK 0x01 + extern ForeignServer *GetForeignServer(Oid serverid); +extern ForeignServer *GetForeignServerExtended(Oid serverid, + bits16 flags); extern ForeignServer *GetForeignServerByName(const char *name, bool missing_ok); extern UserMapping *GetUserMapping(Oid userid, Oid serverid); extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid); +extern ForeignDataWrapper *GetForeignDataWrapperExtended(Oid fdwid, + bits16 flags); extern ForeignDataWrapper *GetForeignDataWrapperByName(const char *name, bool missing_ok); extern ForeignTable *GetForeignTable(Oid relid); -- 2.20.0.rc2
0002-Eliminate-user-visible-cache-lookup-errors-for-objad.patch.gz
Description: application/gzip
signature.asc
Description: PGP signature
