Michael, thanks so much for the review! On July 15, 2015 at 7:35:11 PM, Michael Paquier (michael.paqu...@gmail.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. --
fdw-extension-support-2.diff
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
