Re: [PATCH] postgres_fdw extension support

Back from summer and conferencing, and finally responding, sorry for
the delay...

On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
>
>
> if (needlabel)
> appendStringInfo(buf, "::%s",
> -
> format_type_with_typemod(node->consttype,
> -
> node->consttypmod));
> +
> format_type_be_qualified(node->consttype));
> Pondering more about this one, I think that we are going to need a new
> routine in format_type.c to be able to call format_type_internal as
> format_type_internal(type_oid, typemod, true/false, false, true). If typemod
> is -1, then typemod_given (the third argument) is false, otherwise
> typemod_given is true. That's close to what the C function format_type at
> the SQL level can do except that we want it to be qualified. Regression
> tests will need an update as well.

I ended up switching on whether the type being formatted was an
extension type or not. Extension types need to be fully qualified or
they won't get found by the remote. Conversely if you fully qualify
the built-in types, the regression test for postgres_fdw isn't happy.
This still isn't quite the best/final solution, perhaps something as
simple as this would work, but I'm not sure:

src/backend/utils/adt/format_type.c
+/*
+ * This version allows a nondefault typemod to be specified and fully
qualified.
+ */
+char *
+format_type_with_typemod_qualified(Oid type_oid, int32 typemod)
+{
+ return format_type_internal(type_oid, typemod, true, false, true);
+}

> Comment format is incorrect, this should be written like that:

Comments fixed.

> + if (!extension_list)
> + return false;
> I would rather mark that as == NIL instead, NIL is what an empty list is.

Done

> +extern bool is_shippable(Oid procnumber, PgFdwRelationInfo *fpinfo);
> There is no need to pass PgFdwRelationInfo to is_shippable. Only the list of
> extension OIDs is fine.

Done

> That's nitpicking, but normally we avoid more than 80 characters per line of
> code.

Done.

> When attempting to create a server when an extension is not installed we get
> an appropriate error:
> =# CREATE SERVER postgres_server
> FOREIGN DATA WRAPPER postgres_fdw
> OPTIONS (host 'localhost', port '5432', dbname 'postgres', extensions
> 'seg');
> ERROR: 42601: the "seg" extension must be installed locally before it can
> be used on a remote server
> LOCATION: extractExtensionList, shippable.c:245
> However, it is possible to drop an extension listed in a postgres_fdw
> server. Shouldn't we invalidate cache as well when pg_extension is updated?
> Thoughts?

For extensions with types, it's pretty hard to pull the extension out
from under the FDW, because the tables using the type will depend on
it. For simpler function-only extensions, it's possible, but as soon
as you pull the extension out and run a query, your FDW will notice
the extension is missing and complain. And then you'll have to update
the foreign server or foreign table entries and the cache will get
flushed. So there's no way to get a stale cache.

> + list_free(extlist);
> It does not matter much to call list_free here. The list is going to be
> freed in the error context with ERROR.

Done.

> + foreach(ext, extension_list)
> + {
> + Oid extension_oid = (Oid) lfirst(ext);
> + if (foundDep->refobjid == extension_oid)
> + {
> + nresults++;
> + }
> + }
> You could just use list_member_oid here. And why not just breaking the loop
> once there is a match? This is doing unnecessary work visibly

Done.

> + By default only built-in operators and functions will be sent from the
> + local to the foreign server. This may be overridden using the following
> option:
> Missing a mention to "data types" here?

Actually extension data types traverse the postgres_fdw without
trouble without this patch, as long as both sides have the extension
installed. So not strictly needed to mention data types.

> It would be good to get some regression tests for this feature, which is
> something doable with the flag EXTRA_INSTALL and some tests with EXPLAIN
> VERBOSE. Note that you will need to use CREATE EXTENSION to make the
> extension available in the new test, which should be I imagine a new file
> like shippable.sql.

I've put a very very small regression file in that tests the shippable
feature, which can be fleshed out further as needed.

Thanks so much!

P.

> Regards,
> --
> Michael