Re: [PATCH] postgres_fdw extension support - Mailing list pgsql-hackers
| From | Michael Paquier |
|---|---|
| Subject | Re: [PATCH] postgres_fdw extension support |
| Date | |
| Msg-id | CAB7nPqTa=UfjKggg4OvLNGC7Jz1vZuLoT-qbBZpGbkMXHwUHGA@mail.gmail.com Whole thread Raw |
| In response to | Re: [PATCH] postgres_fdw extension support (Paul Ramsey <pramsey@cleverelephant.ca>) |
| Responses |
Re: [PATCH] postgres_fdw extension support
(Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: [PATCH] postgres_fdw extension support (Paul Ramsey <pramsey@cleverelephant.ca>) |
| List | pgsql-hackers |
On Thu, Jul 23, 2015 at 11:48 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> On Wed, Jul 22, 2015 at 12:19 PM, Paul Ramsey <pramsey@cleverelephant.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));
+ /* Option validation calls this function with NULL in the */
+ /* extensionOids parameter, to just do existence/syntax */
+ /* checking of the option */
+ /*
+ * 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);
+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 */
+ 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
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)));
+ }
+ 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;
+ }
+ 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:
+ 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
Michael
pgsql-hackers by date: