Re: [PATCH] postgres_fdw extension support

On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey <pramsey(at)cleverelephant(dot)ca> wrote:
> Attached is a patch that implements the extension support discussed at
> PgCon this year during the FDW unconference sesssion. Highlights:
>
> * Pass extension operators and functions to the foreign server
> * Only send ops/funcs if the foreign server is declared to support the
> relevant extension, for example:
>
> CREATE SERVER foreign_server
> FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (host '127.0.0.1', port '5432', dbname 'my_db',
> extensions 'cube, seg');
>
> Github branch is here:
> https://github.com/pramsey/postgres/tree/fdw-extension-suppport
>
> Synthetic pull request for easy browsing/commenting is here:
> https://github.com/pramsey/postgres/pull/1

+
/*
* Returns true if given expr is safe to evaluate on the foreign server.
*/
@@ -177,7 +185,7 @@ is_foreign_expr(PlannerInfo *root,
{
foreign_glob_cxt glob_cxt;
foreign_loc_cxt loc_cxt;
-
+
/*
* Check that the expression consists of nodes that are safe to execute
* remotely.
@@ -207,6 +215,8 @@ is_foreign_expr(PlannerInfo *root,
return true;
}

+
+

This patch includes some diff noise, it would be better to remove that.

- if (!is_builtin(fe->funcid))
+ if (!is_builtin(fe->funcid) &&
(!is_in_extension(fe->funcid, fpinfo)))
Extra parenthesis are not needed.

+ if ( (!is_builtin(oe->opno)) &&
(!is_in_extension(oe->opno, fpinfo)) )
... And this does not respect the project code format. See here for
more details for example:
http://www.postgresql.org/docs/devel/static/source.html

+ /* PostGIS metadata */
+ List *extensions;
+ bool use_postgis;
+ Oid postgis_oid;
This addition in PgFdwRelationInfo is surprising. What the point of
keeping use_postgis and postgres_oid that are actually used nowhere?
(And you could rely on the fact that postgis_oid is InvalidOid to
determine if it is defined or not btw). I have a hard time
understanding why this refers to PostGIS as well as a postgres_fdw
feature.

appendStringInfo(buf, "::%s",
- format_type_with_typemod(node->consttype,
- node->consttypmod));
+ format_type_be_qualified(node->consttype));
What's the reason for this change?

Thinking a bit wider, why is this just limited to extensions? You may
have as well other objects defined locally and remotely like functions
or operators that are not defined in extensions, but as normal
objects. Hence I think that what you are looking for is not this
option, but a boolean option enforcing the behavior of code paths
using is_builtin() in foreign_expr_walker such as the sanity checks on
existing objects are not limited to FirstBootstrapObjectId but to
other objects in the catalogs. That's a risky option because it could
lead to inconsistencies among objects, so obviously the default is
false but by documenting correctly the risks of using this option we
may be able to get something integrated (aka be sure that each object
is defined consistently across the servers queried or you'll have
surprising results!). In short, it seems to me that you are taking the
wrong approach.
--
Michael