Re: [PATCH] postgres_fdw extension support

Michael, thanks so much for the review!

On July 15, 2015 at 7:35:11 PM, Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
This patch includes some diff noise, it would be better to remove that. 
Done.

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

+ 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 
I’m sorry, that link doesn’t clarify for me what’s stylistically wrong here (it’s almost all about error messages), could you help (is it the padding around the conditionals? removed that)

+ /* 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? 
Whoops, a couple old pieces from my proof-of-concept survived the conversion to a generic features. Removed.

appendStringInfo(buf, "::%s", 
- format_type_with_typemod(node->consttype, 
- node->consttypmod)); 
+ format_type_be_qualified(node->consttype)); 
What's the reason for this change? 
Good question. As I recall, the very sparse search path the FDW connection makes can sometimes leave remote function failing to find other functions they need, so we want to force the calls to be schema-qualified. Unfortunately there’s no perfect public call for that, ideally it would be return format_type_internal(type_oid, typemod, true, false, true), but there’s no function like that, so I settled for format_type_be_qualified(), which forces qualification at the expense of ignoring the typmod.

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. 
Well, as I see it there’s three broad categories of behavior available:

1- Forward nothing non-built-in (current behavior)

2- Use options to forward only specified non-built-in things (either in function chunks (extensions, as in this patch) or one-by-one (mark your desired functions / ops)

3- Forward everything if a “forward everything” option is set

I hadn’t actually considered the possibility of option 3, but for my purposes it would work just as well, with the added efficiency bonus of not having to check whether particular funcs/ops are inside declared extensions. Both the current state of FDW and the patch I’m providing expect a *bit* of smarts on the part of users, to make sure their remote/local environments are in sync (in particular versions of pgsql and of extensions). Option 3 just ups the ante on that requirement. I’d be fine w/ this, makes the patch very simple and fast.

For option 2, marking things one at a time really isn’t practical for a package like PostGIS, with several hundred functions and operators. Using the larger block of “extension” makes more sense. I think in general, expecting users to itemize every func/op they want to forward, particular if they just want an extension to “work” over FDW is too big an expectation. That’s not to minimize the utility of being able to mark individual functions/ops for forwarding, but I think that’s a separate use case that doesn’t eliminate the need for extension-level forwarding.

Thanks again for the review!

P.

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