Re: [PATCH] postgres_fdw extension support

On Thu, Jul 23, 2015 at 11:48 PM, Paul Ramsey <pramsey(at)cleverelephant(dot)ca>
wrote:
> On Wed, Jul 22, 2015 at 12:19 PM, Paul Ramsey <pramsey(at)cleverelephant(dot)ca>
wrote:
>>
>> I’ll have a look at doing invalidation for the case of changes to the FDW
>> wrappers and servers.
>
> Here's an updated patch that clears the cache on changes to foreign
> wrappers and servers.

Thanks. So I have finally looked at it and this is quite cool. Using for
example this setup:
CREATE EXTENSION seg;
CREATE EXTENSION postgres_fdw;
CREATE SERVER postgres_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host 'localhost', port '5432', dbname 'postgres', extensions 'seg');
CREATE USER MAPPING FOR PUBLIC SERVER postgres_server OPTIONS (password '');
CREATE FOREIGN TABLE aa_foreign (a seg) SERVER postgres_server OPTIONS
(table_name 'aa');
CREATE SERVER postgres_server_2 FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(host 'localhost', port '5432', dbname 'postgres');
CREATE USER MAPPING FOR PUBLIC SERVER postgres_server_2 OPTIONS (password
'');
CREATE FOREIGN TABLE aa_foreign_2 (a seg) SERVER postgres_server_2 OPTIONS
(table_name 'aa');

I can get the following differences by shipping an expression or not:
=# explain verbose select * from aa_foreign where a = '1 ... 2'::seg;
QUERY
PLAN
-------------------------------------------------------------------------------------------
Foreign Scan on public.aa_foreign (cost=100.00..138.66 rows=11 width=12)
Output: a
Remote SQL: SELECT a FROM public.aa WHERE ((a OPERATOR(public.=) '1 ..
2'::public.seg))
(3 rows)
=# explain verbose select * from aa_foreign_2 where a = '1 ... 2'::seg;
QUERY PLAN
-----------------------------------------------------------------------------
Foreign Scan on public.aa_foreign_2 (cost=100.00..182.44 rows=11 width=12)
Output: a
Filter: (aa_foreign_2.a = '1 .. 2'::seg)
Remote SQL: SELECT a FROM public.aa
(4 rows)

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.

+ /* Option validation calls this function with NULL in the */
+ /* extensionOids parameter, to just do existence/syntax */
+ /* checking of the option */
Comment format is incorrect, this should be written like that:
+ /*
+ * Option validation calls this function with NULL in the
+ * extensionOids parameter, to just do existence/syntax
+ * checking of the option.
+ */

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

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

+ bool shippable; /* extension the object appears within, or
InvalidOid if none */
That's nitpicking, but normally we avoid more than 80 characters per line
of code.

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?

+ if (!SplitIdentifierString(extensionString, ',', &extlist))
+ {
+ list_free(extlist);
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("unable to parse extension list \"%s\"",
+ extensionString)));
+ }
It does not matter much to call list_free here. The list is going to be
freed in the error context with ERROR.

+ 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

+ foreach(o, *extensionOids)
+ {
+ Oid oid = (Oid) lfirst(o);
+ if (oid == extension_oid)
+ found = true;
+ }
Here also you could simplify with list_member_oid.

+ 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?

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.
Regards,
--
Michael