Re: [PATCH] postgres_fdw extension support

On Fri, Jul 17, 2015 at 1:08 AM, Paul Ramsey wrote:
> + 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)

Two things:
1) Spaces between parenthesis at the beginning and the end of he expression
2) Parenthesis around is_in_extension are not that much necessary.


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

Hm. I don't have my mind wrapped around that now, but this needs to be
checked with data types like char or varchar. It may matter.

> 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

Then what you are describing here is a parameter able to do a switch
among this selection:
- nothing, which is to check on built-in stuff
- extension list.
- all.

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

Yeah, perhaps too simple though :) You may want something that is more
advanced. For example when using a set of objects you can decide which
want you want to pushdown and which one you want to keep as evaluated
locally.

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

OK, that's good to know. Perhaps then that using a parameter with an
extension list is a good balance. Now would there be practical cases
where it is actually useful to have an extension list? For example,
let's say that there are two extensions installed: foo1 and foo2. Do
you think (or know) if some users would be willing to include only the
objects of foo1, but include those of foo2? Also, could it be possible
to consider an exclude list? Like ignoring all the objects in the
given extension list and allow the rest.
I am just trying to consider all the possibilities here.

> Thanks again for the review!

Good to see that you added in the CF app:
https://commitfest.postgresql.org/6/304/

Regards,
--
Michael