Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Paul Ramsey  writes:
> [ 20151006b_postgres_fdw_extensions.patch ]

Starting to look through this now.  I'm dubious of the decision to have
ExtractExtensionList throw errors if there are un-installed extensions
mentioned in the FDW options.  Wouldn't it be a lot more convenient if
such extension names were silently ignored?  You cannot guarantee that the
list is always up to date anyway; consider creating a server, setting some
extension options, and then dropping some of those extensions.  Moreover,
the current semantics create a hazard for pg_dump, which can't reasonably
be expected to know that it must restore extensions X and Y before it can
create foreign server Z.

There might be a case for raising a WARNING during
postgres_fdw_validator(), but no more than that, IMO.  Certainly ERROR
during regular use of the server is right out.

Comments?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Robert Haas]

On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane  wrote:
> Paul Ramsey  writes:
>> [ 20151006b_postgres_fdw_extensions.patch ]
>
> Starting to look through this now.  I'm dubious of the decision to have
> ExtractExtensionList throw errors if there are un-installed extensions
> mentioned in the FDW options.  Wouldn't it be a lot more convenient if
> such extension names were silently ignored?  You cannot guarantee that the
> list is always up to date anyway; consider creating a server, setting some
> extension options, and then dropping some of those extensions.  Moreover,
> the current semantics create a hazard for pg_dump, which can't reasonably
> be expected to know that it must restore extensions X and Y before it can
> create foreign server Z.
>
> There might be a case for raising a WARNING during
> postgres_fdw_validator(), but no more than that, IMO.  Certainly ERROR
> during regular use of the server is right out.

Agreed.  I don't know whether it's better to emit a WARNING or some
lower-level message (INFO, DEBUG), but I think an ERROR will suck due
to the pg_dump issues you mention.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Robert Haas  writes:
> On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane  wrote:
>> Paul Ramsey  writes:
>>> [ 20151006b_postgres_fdw_extensions.patch ]

>> There might be a case for raising a WARNING during
>> postgres_fdw_validator(), but no more than that, IMO.  Certainly ERROR
>> during regular use of the server is right out.

> Agreed.  I don't know whether it's better to emit a WARNING or some
> lower-level message (INFO, DEBUG), but I think an ERROR will suck due
> to the pg_dump issues you mention.

I've committed this with a WARNING during validation and no comment
otherwise.

I left out the proposed regression tests because they fail in "make
installcheck" mode, unless you've previously built and installed cube
and seg, which seems like an unacceptable requirement to me.  I don't
think that leaving the code untested is a good final answer, of course.
The idea I was toying with was to create a dummy extension for testing
purposes by means of doing a direct INSERT into pg_extension --- which
is ugly and would only work for superusers, but the latter is true of
"CREATE EXTENSION cube" too.  Anybody have a better idea?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

Thanks everyone for the held and feedback on this patch!

-- 
Paul Ramsey
http://cleverelephant.ca
http://postgis.net

On November 3, 2015 at 3:47:37 PM, Tom Lane (t...@sss.pgh.pa.us) wrote:

Robert Haas  writes:  
> On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane  wrote:  
>> Paul Ramsey  writes:  
>>> [ 20151006b_postgres_fdw_extensions.patch ]  

>> There might be a case for raising a WARNING during  
>> postgres_fdw_validator(), but no more than that, IMO. Certainly ERROR  
>> during regular use of the server is right out.  

> Agreed. I don't know whether it's better to emit a WARNING or some  
> lower-level message (INFO, DEBUG), but I think an ERROR will suck due  
> to the pg_dump issues you mention.  

I've committed this with a WARNING during validation and no comment  
otherwise.  

I left out the proposed regression tests because they fail in "make  
installcheck" mode, unless you've previously built and installed cube  
and seg, which seems like an unacceptable requirement to me. I don't  
think that leaving the code untested is a good final answer, of course.  
The idea I was toying with was to create a dummy extension for testing  
purposes by means of doing a direct INSERT into pg_extension --- which  
is ugly and would only work for superusers, but the latter is true of  
"CREATE EXTENSION cube" too. Anybody have a better idea?  

regards, tom lane  

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

I wrote:
> I left out the proposed regression tests because they fail in "make
> installcheck" mode, unless you've previously built and installed cube
> and seg, which seems like an unacceptable requirement to me.  I don't
> think that leaving the code untested is a good final answer, of course.
> The idea I was toying with was to create a dummy extension for testing
> purposes by means of doing a direct INSERT into pg_extension --- which
> is ugly and would only work for superusers, but the latter is true of
> "CREATE EXTENSION cube" too.  Anybody have a better idea?

I had a possibly better idea: instead of manufacturing an empty extension
with a direct INSERT, hack on the one extension that we know for sure
will be installed, namely postgres_fdw itself.  So we could do, eg,

create function foo() ...
alter extension postgres_fdw add function foo();

and then test shippability of foo() with or without having listed
postgres_fdw as a shippable extension.

This is certainly pretty ugly in its own right, but it would avoid
the maintainability hazards of an explicit INSERT into pg_extension.
So on balance it seems a bit nicer than my first idea.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Wed, Nov 4, 2015 at 2:23 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> That's still strange to have a dummy object in
>> postgres_fdw.so just for testing purposes.
>
> We could drop the extra functions at the end of the test, but I don't
> see the point exactly.  We'd just be leaving the regression test database
> with some odd contents of the extension --- there's not any wider effect
> than that.

Oh, I see. Then we rely just on the fact that postgres_fdw is
installed. Are you working on a patch? Do you need one?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Wed, Nov 4, 2015 at 12:38 PM, Tom Lane wrote:
> I wrote:
>> I left out the proposed regression tests because they fail in "make
>> installcheck" mode, unless you've previously built and installed cube
>> and seg, which seems like an unacceptable requirement to me.  I don't
>> think that leaving the code untested is a good final answer, of course.
>> The idea I was toying with was to create a dummy extension for testing
>> purposes by means of doing a direct INSERT into pg_extension --- which
>> is ugly and would only work for superusers, but the latter is true of
>> "CREATE EXTENSION cube" too.  Anybody have a better idea?
>
> I had a possibly better idea: instead of manufacturing an empty extension
> with a direct INSERT, hack on the one extension that we know for sure
> will be installed, namely postgres_fdw itself.  So we could do, eg,
>
> create function foo() ...
> alter extension postgres_fdw add function foo();
> and then test shippability of foo() with or without having listed
> postgres_fdw as a shippable extension.

Yeah, I don't have a better idea than that. Could we consider shipping
that in a different library than postgres_fdw.so, like
postgres_fdw_test.so? That's still strange to have a dummy object in
postgres_fdw.so just for testing purposes.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Michael Paquier  writes:
> On Wed, Nov 4, 2015 at 12:38 PM, Tom Lane wrote:
>> I had a possibly better idea: instead of manufacturing an empty extension
>> with a direct INSERT, hack on the one extension that we know for sure
>> will be installed, namely postgres_fdw itself.  So we could do, eg,
>> 
>> create function foo() ...
>> alter extension postgres_fdw add function foo();
>> and then test shippability of foo() with or without having listed
>> postgres_fdw as a shippable extension.

> Yeah, I don't have a better idea than that. Could we consider shipping
> that in a different library than postgres_fdw.so, like
> postgres_fdw_test.so?

I'm envisioning the extra function(s) as just being SQL functions, so
they don't need any particular infrastructure.

> That's still strange to have a dummy object in
> postgres_fdw.so just for testing purposes.

We could drop the extra functions at the end of the test, but I don't
see the point exactly.  We'd just be leaving the regression test database
with some odd contents of the extension --- there's not any wider effect
than that.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Tue, Oct 6, 2015 at 10:32 PM, Andres Freund wrote:
> Maybe I'm missing something major here. But given that you're looking up
> solely based on Oid objnumber, Oid classnumber, how would this cache
> work if there's two foreign servers with different extension lists?

Oh. Nice catch here.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-10-06 07:01:53 -0700, Paul Ramsey wrote:
> diff --git a/contrib/postgres_fdw/sql/shippable.sql 
> b/contrib/postgres_fdw/sql/shippable.sql
> new file mode 100644
> index 000..83ee38c
> --- /dev/null
> +++ b/contrib/postgres_fdw/sql/shippable.sql
> @@ -0,0 +1,76 @@

I think it'd be good to add a test exercising two servers with different
extensions marked as shippable.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Tue, Oct 6, 2015 at 8:52 AM, Andres Freund  wrote:
> I think it'd be good to add a test exercising two servers with different
> extensions marked as shippable.

Done,
P


20151006b_postgres_fdw_extensions.patch
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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

 
On October 4, 2015 at 9:56:10 PM, Michael Paquier 
(michael.paqu...@gmail.com(mailto:michael.paqu...@gmail.com)) wrote:
> On Sun, Oct 4, 2015 at 11:40 AM, Paul Ramsey wrote:
> > I put all changes relative to your review here if you want a nice colorized
> > place to check
> >
> > https://github.com/pramsey/postgres/commit/ed33e7489601e659f436d6afda3cce28304eba50
>  
> - /* updatable is available on both server and table */
> + /* updatable option is available on both server and table */
> This is just noise (perhaps I am the one who introduced it, oops).

No, I did, it matches the change to the comment below that addresses Andres 
note. The principle of least change is butting up against the principle of 
language usage consistency here. Can remove, of course.

P 


--  
http://postgis.net  
http://cleverelephant.ca




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Tue, Oct 6, 2015 at 5:32 AM, Andres Freund  wrote:

> The problem is basically that cache invalidations can arrive while
> you're building new cache entries. Everytime you e.g. open a relation
> cache invalidations can arrive. Assume you'd written the code like:

> You're avoiding that by only entering into the hashtable *after* the
> lookup. And I think that deserves a comment.

Thanks, revised patch attached.

P.


20151006_postgres_fdw_extensions.patch
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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-10-03 19:40:40 -0700, Paul Ramsey wrote:
> > > +  /* 
> > > +  * Right now "shippability" is exclusively a function of whether 
> > > +  * the obj (proc/op/type) is in an extension declared by the user. 
> > > +  * In the future we could additionally have a whitelist of functions 
> > > +  * declared one at a time. 
> > > +  */ 
> > > +  bool shippable = lookup_shippable(objnumber, extension_list); 
> > > + 
> > > +  entry = (ShippableCacheEntry *) 
> > > +  hash_search(ShippableCacheHash, 
> > > +  (void *) , 
> > > +  HASH_ENTER, 
> > > +  NULL); 
> > > + 
> > > +  entry->shippable = shippable; 
> > > + } 
> > 
> > I suggest adding a comment that we consciously are only HASH_ENTERing 
> > the key after doing the lookup_shippable(), to avoid the issue that the 
> > lookup might accept cache invalidations and thus delete the entry again. 

> I’m not understanding this one. I only ever delete cache entries in
> bulk, when InvalidateShippableCacheCallback gets called on updates to
> the foreign server definition. I must not be quite understanding your
> gist.

The problem is basically that cache invalidations can arrive while
you're building new cache entries. Everytime you e.g. open a relation
cache invalidations can arrive. Assume you'd written the code like:

entry = hash_search(HASH_ENTER, key, );

if (found)
return entry->whateva;

entry->whateva = lookup_shippable(...);
return entry->whateva;

lookup_shippable() opens a relation, which accepts invalidations. Which
then go and delete the contents of the hashtable. And you're suddenly
accessing free()d memory...

You're avoiding that by only entering into the hashtable *after* the
lookup. And I think that deserves a comment.

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-10-06 06:42:17 -0700, Paul Ramsey wrote:
> *sigh*, no you’re not missing anything. In moving to the cache and
> marking things just as “shippable” I’ve lost the test that ensures
> they are shippable for this *particular* server (which only happens in
> the lookup stage). So yeah, the cache will be wrong in cases where
> different servers have different extension opti ons.

Should be easy enough to fix - just add the server's oid to the key.

But I guess you're already on that.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-10-06 06:28:34 -0700, Paul Ramsey wrote:
> +/*
> + * is_shippable
> + * Is this object (proc/op/type) shippable to foreign server?
> + * Check cache first, then look-up whether (proc/op/type) is
> + * part of a declared extension if it is not cached.
> + */
> +bool
> +is_shippable(Oid objnumber, Oid classnumber, List *extension_list)
> +{
> + ShippableCacheKey key;
> + ShippableCacheEntry *entry;
> +
> + /* Always return false if the user hasn't set the "extensions" option */
> + if (extension_list == NIL)
> + return false;
> +
> + /* Find existing cache, if any. */
> + if (!ShippableCacheHash)
> + InitializeShippableCache();
> +
> + /* Zero out the key */
> + memset(, 0, sizeof(key));
> +
> + key.objid = objnumber;
> + key.classid = classnumber;
> +
> + entry = (ShippableCacheEntry *)
> +  hash_search(ShippableCacheHash,
> + (void *) ,
> + HASH_FIND,
> + NULL);
> +
> + /* Not found in ShippableCacheHash cache.  Construct new entry. */
> + if (!entry)
> + {
> + /*
> +  * Right now "shippability" is exclusively a function of whether
> +  * the obj (proc/op/type) is in an extension declared by the 
> user.
> +  * In the future we could additionally have a whitelist of 
> functions
> +  * declared one at a time.
> +  */
> + bool shippable = lookup_shippable(objnumber, classnumber, 
> extension_list);
> +
> + /*
> +  * Don't create a new hash entry until *after* we have the 
> shippable
> +  * result in hand, as the shippable lookup might trigger a cache
> +  * invalidation.
> +  */
> + entry = (ShippableCacheEntry *)
> +  hash_search(ShippableCacheHash,
> + (void *) ,
> + HASH_ENTER,
> + NULL);
> +
> + entry->shippable = shippable;
> + }
> +
> + if (!entry)
> + return false;
> + else
> + return entry->shippable;
> +}

Maybe I'm missing something major here. But given that you're looking up
solely based on Oid objnumber, Oid classnumber, how would this cache
work if there's two foreign servers with different extension lists?

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On October 6, 2015 at 6:32:36 AM, Andres Freund 
(and...@anarazel.de(mailto:and...@anarazel.de)) wrote:

> On 2015-10-06 06:28:34 -0700, Paul Ramsey wrote:
> > +/*
> > + * is_shippable
> > + * Is this object (proc/op/type) shippable to foreign server?
> > + * Check cache first, then look-up whether (proc/op/type) is
> > + * part of a declared extension if it is not cached.
> > + */
> > +bool
> > +is_shippable(Oid objnumber, Oid classnumber, List *extension_list)
> > +{
> > + ShippableCacheKey key;
> > + ShippableCacheEntry *entry;
> > +
> > + /* Always return false if the user hasn't set the "extensions" option */
> > + if (extension_list == NIL)
> > + return false;
> > +
> > + /* Find existing cache, if any. */
> > + if (!ShippableCacheHash)
> > + InitializeShippableCache();
> > +
> > + /* Zero out the key */
> > + memset(, 0, sizeof(key));
> > +
> > + key.objid = objnumber;
> > + key.classid = classnumber;
> > +
> > + entry = (ShippableCacheEntry *)
> > + hash_search(ShippableCacheHash,
> > + (void *) ,
> > + HASH_FIND,
> > + NULL);
> > +
> > + /* Not found in ShippableCacheHash cache. Construct new entry. */
> > + if (!entry)
> > + {
> > + /*
> > + * Right now "shippability" is exclusively a function of whether
> > + * the obj (proc/op/type) is in an extension declared by the user.
> > + * In the future we could additionally have a whitelist of functions
> > + * declared one at a time.
> > + */
> > + bool shippable = lookup_shippable(objnumber, classnumber, extension_list);
> > +
> > + /*
> > + * Don't create a new hash entry until *after* we have the shippable
> > + * result in hand, as the shippable lookup might trigger a cache
> > + * invalidation.
> > + */
> > + entry = (ShippableCacheEntry *)
> > + hash_search(ShippableCacheHash,
> > + (void *) ,
> > + HASH_ENTER,
> > + NULL);
> > +
> > + entry->shippable = shippable;
> > + }
> > +
> > + if (!entry)
> > + return false;
> > + else
> > + return entry->shippable;
> > +}
>  
> Maybe I'm missing something major here. But given that you're looking up
> solely based on Oid objnumber, Oid classnumber, how would this cache
> work if there's two foreign servers with different extension lists?

*sigh*, no you’re not missing anything. In moving to the cache and marking 
things just as “shippable” I’ve lost the test that ensures they are shippable 
for this *particular* server (which only happens in the lookup stage). So yeah, 
the cache will be wrong in cases where different servers have different 
extension opti ons.

Thanks, 
P.


--  
http://postgis.net  
http://cleverelephant.ca




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Tue, Oct 6, 2015 at 6:55 AM, Andres Freund  wrote:
> On 2015-10-06 06:42:17 -0700, Paul Ramsey wrote:
>> *sigh*, no you’re not missing anything. In moving to the cache and
>> marking things just as “shippable” I’ve lost the test that ensures
>> they are shippable for this *particular* server (which only happens in
>> the lookup stage). So yeah, the cache will be wrong in cases where
>> different servers have different extension opti ons.
>
> Should be easy enough to fix - just add the server's oid to the key.
>
> But I guess you're already on that.

Yep, just a big chastened :)
Thanks for the catch, here's the patch,
P


20151006a_postgres_fdw_extensions.patch
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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Sun, Oct 4, 2015 at 11:40 AM, Paul Ramsey  wrote:
> I put all changes relative to your review here if you want a nice colorized
> place to check
>
> https://github.com/pramsey/postgres/commit/ed33e7489601e659f436d6afda3cce28304eba50

-/* updatable is available on both server and table */
+/* updatable option is available on both server and table */
This is just noise (perhaps I am the one who introduced it, oops).
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

Hi,

On 2015-10-01 11:46:43 +0900, Michael Paquier wrote:
> diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
> index 7547ec2..864bf53 100644
> --- a/contrib/postgres_fdw/option.c
> +++ b/contrib/postgres_fdw/option.c
> @@ -19,6 +19,8 @@
>  #include "catalog/pg_foreign_table.h"
>  #include "catalog/pg_user_mapping.h"
>  #include "commands/defrem.h"
> +#include "commands/extension.h"
> +#include "utils/builtins.h"
>  
>  
>  /*
> @@ -124,6 +126,11 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
>errmsg("%s requires a 
> non-negative numeric value",
>   def->defname)));
>   }
> + else if (strcmp(def->defname, "extensions") == 0)
> + {
> + /* this must have already-installed extensions */

I don't understand that comment.

>   PG_RETURN_VOID();
> @@ -153,6 +160,8 @@ InitPgFdwOptions(void)
>   /* updatable is available on both server and table */
>   {"updatable", ForeignServerRelationId, false},
>   {"updatable", ForeignTableRelationId, false},
> + /* extensions is available on server */
> + {"extensions", ForeignServerRelationId, false},

awkward spelling in comment.


> +/*
> + * Parse a comma-separated string and return a List of the Oids of the
> + * extensions in the string. If an extension provided cannot be looked
> + * up in the catalog (it hasn't been installed or doesn't exist) then
> + * throw up an error.
> + */

s/throw up an error/throw an error/ or raise an error.

> +/*
> + * FDW-specific planner information kept in RelOptInfo.fdw_private for a
> + * foreign table.  This information is collected by 
> postgresGetForeignRelSize.
> + */
> +typedef struct PgFdwRelationInfo
> +{
> + /* baserestrictinfo clauses, broken down into safe and unsafe subsets. 
> */
> + List   *remote_conds;
> + List   *local_conds;
> +
> + /* Bitmap of attr numbers we need to fetch from the remote server. */
> + Bitmapset  *attrs_used;
> +
> + /* Cost and selectivity of local_conds. */
> + QualCostlocal_conds_cost;
> + Selectivity local_conds_sel;
> +
> + /* Estimated size and cost for a scan with baserestrictinfo quals. */
> + double  rows;
> + int width;
> + Coststartup_cost;
> + Costtotal_cost;
> +
> + /* Options extracted from catalogs. */
> + booluse_remote_estimate;
> + Costfdw_startup_cost;
> + Costfdw_tuple_cost;
> +
> + /* Optional extensions to support (list of oid) */

*oids

> +/* objid is the lookup key, must appear first */
> +typedef struct
> +{
> + /* extension the object appears within, or InvalidOid if none */
> + Oid objid;
> +} ShippableCacheKey;
> +
> +typedef struct
> +{
> + /* lookup key - must be first */
> + ShippableCacheKey key;
> + bool shippable;
> +} ShippableCacheEntry;
> +
> +/*
> + * Flush all cache entries when pg_foreign_server is updated.
> + */
> +static void
> +InvalidateShippableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
> +{
> + HASH_SEQ_STATUS status;
> + ShippableCacheEntry *entry;
> +
> + hash_seq_init(, ShippableCacheHash);
> + while ((entry = (ShippableCacheEntry *) hash_seq_search()) != 
> NULL)
> + {
> + if (hash_search(ShippableCacheHash,
> + (void *) >key,
> + HASH_REMOVE,
> + NULL) == NULL)
> + elog(ERROR, "hash table corrupted");
> + }
> +}

> +/*
> + * Returns true if given operator/function is part of an extension declared 
> in
> + * the server options.
> + */
> +static bool
> +lookup_shippable(Oid objnumber, List *extension_list)
> +{
> + static int nkeys = 1;
> + ScanKeyData key[nkeys];
> + HeapTuple tup;
> + Relation depRel;
> + SysScanDesc scan;
> + bool is_shippable = false;
> +
> + /* Always return false if we don't have any declared extensions */

Imo there's nothing declared here...

> + if (extension_list == NIL)
> + return false;
> +
> + /* We need this relation to scan */

Not sure what that means.

> + depRel = heap_open(DependRelationId, RowExclusiveLock);
> +
> + /*
> +  * Scan the system dependency table for all entries this object
> +  * depends on, then iterate through and see if one of them
> +  * is an extension declared by the user in the options
> +  */
> + ScanKeyInit([0],
> + Anum_pg_depend_objid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(objnumber));
> +
> + scan = systable_beginscan(depRel, DependDependerIndexId, true,
> +

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

Andres, 
Thanks so much for the review!

I put all changes relative to your review here if you want a nice colorized 
place to check

https://github.com/pramsey/postgres/commit/ed33e7489601e659f436d6afda3cce28304eba50

On October 3, 2015 at 8:49:04 AM, Andres Freund (and...@anarazel.de) wrote:

> +  /* this must have already-installed extensions */ 

I don't understand that comment. 
Fixed, I hope.

> +  /* extensions is available on server */ 
> +  {"extensions", ForeignServerRelationId, false}, 

awkward spelling in comment. 
Fixed, I hope.

> + * throw up an error. 
> + */ 

s/throw up an error/throw an error/ or raise an error. 
But “throw up” is so evocative :) fixed.

> + /* Optional extensions to support (list of oid) */ 

*oids 
Fixed.

> + /* Always return false if we don't have any declared extensions */ 

Imo there's nothing declared here... 
Changed...

> + if (extension_list == NIL) 
> +  return false; 
> + 
> + /* We need this relation to scan */ 

Not sure what that means. 
Me neither, removed.


> +  if (foundDep->deptype == DEPENDENCY_EXTENSION && 
> +  list_member_oid(extension_list, foundDep->refobjid)) 
> +  { 
> +  is_shippable = true; 
> +  break; 
> +  } 
> + } 

Hm. 
I think this “hm” is addressed lower down.

> + /* Always return false if we don't have any declared extensions */ 
> + if (extension_list == NIL) 
> +  return false; 

I again dislike declared here ;) 
Altered.


> + key.objid = objnumber; 

Hm. Oids can conflict between different system relations. Shouldn't the 
key be over class (i.e. pg_proc, pg_type etc.) and object id? 
I’ve changed the lookup to use class/obj instead. I’m *hoping* I don’t get 
burned by it, but it regresses fine at least. Each call to is_shippable now has 
a hard-coded class oid in it depending on the context of the call. It seemed 
like the right way to do it.

> +  /* 
> +  * Right now "shippability" is exclusively a function of whether 
> +  * the obj (proc/op/type) is in an extension declared by the user. 
> +  * In the future we could additionally have a whitelist of functions 
> +  * declared one at a time. 
> +  */ 
> +  bool shippable = lookup_shippable(objnumber, extension_list); 
> + 
> +  entry = (ShippableCacheEntry *) 
> +  hash_search(ShippableCacheHash, 
> +  (void *) , 
> +  HASH_ENTER, 
> +  NULL); 
> + 
> +  entry->shippable = shippable; 
> + } 

I suggest adding a comment that we consciously are only HASH_ENTERing 
the key after doing the lookup_shippable(), to avoid the issue that the 
lookup might accept cache invalidations and thus delete the entry again. 
I’m not understanding this one. I only ever delete cache entries in bulk, when 
InvalidateShippableCacheCallback gets called on updates to the foreign server 
definition. I must not be quite understanding your gist.

Thanks!

P






20151003_postgres_fdw_extensions.patch
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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Mon, Sep 28, 2015 at 10:47 PM, Paul Ramsey  wrote:
> On Sat, Sep 26, 2015 at 5:41 AM, Michael Paquier
>  wrote:
>> On Sat, Sep 26, 2015 at 4:29 AM, Paul Ramsey wrote:
>>> On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier wrote:
>>> 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);
>>> +}
>>
>> Patch 0001 in this email has a routine called format_type_detailed
>> that I think is basically what we need for this stuff:
>> http://www.postgresql.org/message-id/CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE=mxvrhefjj51ac...@mail.gmail.com
>> Could you look at it?
>
> I'm not sure it helps much. I'd still need a function to turn the
> output into a formatted string, but more importantly the big question
> for me to avoid breaking regression is: if it's built-in, don't schema
> qualitfy it; if it's extended do so. I'm not seeing why ignoring the
> typmod in the case of deparsing extended type constants is going to be
> a problem? All the old behaviour is untouched in the current patch.

Yeah, let's do it so then.

>> +extern bool extractExtensionList(char *extensionString,
>> +   List **extensionOids);
>> What's the point of the boolean status in this new routine? The return
>> value of extractExtensionList is never checked, and it would be more
>> simple to just return the parsed list as return value, no?
>
> I started changing it, then found out why it is the way it is. During
> the options parsing, the list of current extensionOids is passed in,
> so that extra ones can be added, since both the wrapper and the server
> can be declared with extensionOids. It's also doubling as a flag on
> whether the function should bother to try and populate the list, or
> just do a sanity check on the options string. I can change the
> signature to
>
> extern List* extractExtensionList(char *extensionString, List
> *currentExtensionOids, bool populateList);
> to be more explicit if necessary.

Hm. Wouldn't it be just fine if only the server is able to define a
list of extensions then? It seems to me that all the use-cases of this
feature require to have a list of extensions defined per server, and
not per fdw type. This would remove a level of complexity in your
patch without impacting the feature usability as well. I would
personally go without it but I am fine to let a committer (Tom?) put a
final judgement stamp on this matter. Thoughts?

>> +-- ===
>> +-- clean up
>> +-- ===
>> Perhaps here you meant dropping the schema and the foreign tables
>> created previously?
>
> I did, but since postgres_fdw doesn't clean up after itself, perhaps
> just leaving the room messy is the appropriate behaviour?

That's appropriate when keeping around objects that are aimed to be
tested with for example pg_upgrade, some regression tests of
src/test/regress do that on purpose for example. Now it is true that
an extension regression database will be normally reused by another
extension just after, and that there is no general rule on this
matter. Some contrib/ modules do this cleanup, others not. I would
have cleaned up the room if I would have coded this set of tests, but
as you're putting your hands in it I am fine with your decision and
that's really no big deal.

>> +CREATE SCHEMA "SH 1";
>> Is there a reason to use double-quoted relations? There is no real
>> reason to not use them, this is just to point out that this is not a
>> common practice in the other regression tests.
>
> Just following the pattern from postgres_fdw. And since I found things
> to be sensitive to full qualification of function names, etc, it
> seemed like a good idea to try out odd named tables/schemas since the
> pattern was there to follow.

OK. I have no arguments against that.

> I also changed the extension being tested from 'seg' to 'cube', since
> cube had some functions I could test as well as operators.

This change looks fine to me.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On September 30, 2015 at 7:06:58 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:
Paul Ramsey  writes: 
> Hm. Wouldn't it be just fine if only the server is able to define a  
> list of extensions then? It seems to me that all the use-cases of this  
> feature require to have a list of extensions defined per server, and  
> not per fdw type. This would remove a level of complexity in your  
> patch without impacting the feature usability as well. I would  
> personally go without it but I am fine to let a committer (Tom?) put a  
> final judgement stamp on this matter. Thoughts?  

Maybe I'm missing something, but I had envisioned the set of 
safe-to-transmit extensions as typically being defined at the 
foreign-server level. The reason being that you are really declaring two 
things: one is that the extension's operations are reproducible remotely, 
and the other is that the extension is in fact installed on this 
particular remote server. Perhaps there are use-cases for specifying it 
as an FDW option or per-table option, but per-server option seems by 
far the most plausible case. 
Fair enough. Declaring it for the whole database as an option to CREATE FOREIGN 
DATA WRAPPER was just a convenience really, so you could basically say “I 
expect this extension on all my servers”. But you’re right, logically “having 
the extension” is an attribute of the servers, so restricting it to the server 
definitions only has a nice simple logic to it.

P. 


-- 
http://postgis.net
http://cleverelephant.ca

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Thu, Oct 1, 2015 at 4:33 AM, Paul Ramsey  wrote:
>
>
> On September 30, 2015 at 7:06:58 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:
>
> I wrote:
> > Hm. Wouldn't it be just fine if only the server is able to define a
> > list of extensions then? It seems to me that all the use-cases of this
> > feature require to have a list of extensions defined per server, and
> > not per fdw type. This would remove a level of complexity in your
> > patch without impacting the feature usability as well. I would
> > personally go without it but I am fine to let a committer (Tom?) put a
> > final judgement stamp on this matter. Thoughts?
>
> Maybe I'm missing something, but I had envisioned the set of
> safe-to-transmit extensions as typically being defined at the
> foreign-server level. The reason being that you are really declaring two
> things: one is that the extension's operations are reproducible remotely,
> and the other is that the extension is in fact installed on this
> particular remote server. Perhaps there are use-cases for specifying it
> as an FDW option or per-table option, but per-server option seems by
> far the most plausible case.
>
> Fair enough. Declaring it for the whole database as an option to CREATE 
> FOREIGN DATA WRAPPER was just a convenience really, so you could basically 
> say “I expect this extension on all my servers”. But you’re right, logically 
> “having the extension” is an attribute of the servers, so restricting it to 
> the server definitions only has a nice simple logic to it.

OK. Once you can get a new patch done with a reworked
extractExtensionList, I'll get a new look at it in a timely fashion
and then let's move it to a committer's hands.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

 On September 30, 2015 at 3:32:21 PM, Michael Paquier 
(michael.paqu...@gmail.com) wrote:
OK. Once you can get a new patch done with a reworked  
extractExtensionList, I'll get a new look at it in a timely fashion  
and then let's move it to a committer's hands.  

Done and thanks!
P

-- 
http://postgis.net
http://cleverelephant.ca



fdw-extension-support8.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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Thu, Oct 1, 2015 at 7:57 AM, Paul Ramsey wrote:
>  On September 30, 2015 at 3:32:21 PM, Michael Paquier wrote:
>
> OK. Once you can get a new patch done with a reworked
> extractExtensionList, I'll get a new look at it in a timely fashion
> and then let's move it to a committer's hands.

So, I had a final look at that, and finished with the attached. I have
done mainly cosmetic changes such as adjusting some error messages,
correcting the documentation that still referred to FDW supporting the
option "extensions", rewording things. Also, it seems to me that it
makes more sense to move extractExtensionList in option.c. You have as
well forgotten to remove a couple of things related to the previous
FDW interface:
- In InitializeShippableCache, there is no need to check for
FOREIGNDATAWRAPPEROID
- PgFdwRelationInfo does not need wrapper
- postgresGetForeignRelSize does not need to set up the wrapper OID by
calling GetForeignDataWrapper

I also had shared feelings about exposing PgFdwRelationInfo in
postgres_fdw.h, but your approach looks to be the cleanest way because
the extension list is available in the global context through
foreign_rel when checking the shippability of an expression. And I
guess that we definitely do not want to pass the extension list in a
bunch of routines, like appendWhereClause, is_foreign_expr,
classifyConditions, etc. That would be bug-prone thinking long-term.

This patch is switched as "ready for committer".
Regards,
-- 
Michael
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index d2b98e1..f78fc64 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -1,7 +1,7 @@
 # contrib/postgres_fdw/Makefile
 
 MODULE_big = postgres_fdw
-OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES)
+OBJS = postgres_fdw.o option.o deparse.o connection.o shippable.o $(WIN32RES)
 PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL"
 
 PG_CPPFLAGS = -I$(libpq_srcdir)
@@ -10,7 +10,9 @@ SHLIB_LINK = $(libpq)
 EXTENSION = postgres_fdw
 DATA = postgres_fdw--1.0.sql
 
-REGRESS = postgres_fdw
+# Note: shippable tests depend on postgres_fdw tests setup
+REGRESS = postgres_fdw shippable
+EXTRA_INSTALL = contrib/cube
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 697de60..4c06e66 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -233,6 +233,9 @@ foreign_expr_walker(Node *node,
 	Oid			collation;
 	FDWCollateState state;
 
+	/* Access extension metadata from fpinfo on baserel */
+	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)(glob_cxt->foreignrel->fdw_private);
+
 	/* Need do nothing for empty subexpressions */
 	if (node == NULL)
 		return true;
@@ -378,7 +381,8 @@ foreign_expr_walker(Node *node,
  * can't be sent to remote because it might have incompatible
  * semantics on remote side.
  */
-if (!is_builtin(fe->funcid))
+if (!is_builtin(fe->funcid) &&
+	!is_shippable(fe->funcid, fpinfo->extensions))
 	return false;
 
 /*
@@ -426,7 +430,8 @@ foreign_expr_walker(Node *node,
  * (If the operator is, surely its underlying function is
  * too.)
  */
-if (!is_builtin(oe->opno))
+if (!is_builtin(oe->opno) &&
+	!is_shippable(oe->opno, fpinfo->extensions))
 	return false;
 
 /*
@@ -466,7 +471,8 @@ foreign_expr_walker(Node *node,
 /*
  * Again, only built-in operators can be sent to remote.
  */
-if (!is_builtin(oe->opno))
+if (!is_builtin(oe->opno) &&
+	!is_shippable(oe->opno, fpinfo->extensions))
 	return false;
 
 /*
@@ -616,7 +622,9 @@ foreign_expr_walker(Node *node,
 	 * If result type of given expression is not built-in, it can't be sent to
 	 * remote because it might have incompatible semantics on remote side.
 	 */
-	if (check_type && !is_builtin(exprType(node)))
+	if (check_type &&
+		!is_builtin(exprType(node)) &&
+		!is_shippable(exprType(node), fpinfo->extensions))
 		return false;
 
 	/*
@@ -1351,6 +1359,9 @@ deparseConst(Const *node, deparse_expr_cxt *context)
 	bool		isfloat = false;
 	bool		needlabel;
 
+	/* Access extension metadata from fpinfo on baserel */
+	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *)(context->foreignrel->fdw_private);
+
 	if (node->constisnull)
 	{
 		appendStringInfoString(buf, "NULL");
@@ -1428,9 +1439,16 @@ deparseConst(Const *node, deparse_expr_cxt *context)
 			break;
 	}
 	if (needlabel)
+	{
+		/*
+		 * References to extension types need to be fully qualified,
+		 * but references to built-in types shouldn't be.
+		 */
 		appendStringInfo(buf, "::%s",
-		 format_type_with_typemod(node->consttype,
-  node->consttypmod));
+			is_shippable(node->consttype, fpinfo->extensions) ?
+			format_type_be_qualified(node->consttype) :
+			format_type_with_typemod(node->consttype, node->consttypmod));
+	}
 }
 
 /*
diff --git a/contrib/postgres_fdw/expected/shippable.out 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Paul Ramsey  writes:
> Hm. Wouldn't it be just fine if only the server is able to define a 
> list of extensions then? It seems to me that all the use-cases of this 
> feature require to have a list of extensions defined per server, and 
> not per fdw type. This would remove a level of complexity in your 
> patch without impacting the feature usability as well. I would 
> personally go without it but I am fine to let a committer (Tom?) put a 
> final judgement stamp on this matter. Thoughts? 

Maybe I'm missing something, but I had envisioned the set of
safe-to-transmit extensions as typically being defined at the
foreign-server level.  The reason being that you are really declaring two
things: one is that the extension's operations are reproducible remotely,
and the other is that the extension is in fact installed on this
particular remote server.  Perhaps there are use-cases for specifying it
as an FDW option or per-table option, but per-server option seems by
far the most plausible case.

Also, isn't this whole thing specific to postgres_fdw anyway?  I don't
follow your reference to fdw type.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On September 30, 2015 at 12:54:44 AM, Michael Paquier 
(michael.paqu...@gmail.com) wrote:

>> +extern bool extractExtensionList(char *extensionString, 
>> + List **extensionOids); 
>> What's the point of the boolean status in this new routine? The return 
>> value of extractExtensionList is never checked, and it would be more 
>> simple to just return the parsed list as return value, no? 
> 
> I started changing it, then found out why it is the way it is. During 
> the options parsing, the list of current extensionOids is passed in, 
> so that extra ones can be added, since both the wrapper and the server 
> can be declared with extensionOids. It's also doubling as a flag on 
> whether the function should bother to try and populate the list, or 
> just do a sanity check on the options string. I can change the 
> signature to 
> 
> extern List* extractExtensionList(char *extensionString, List 
> *currentExtensionOids, bool populateList); 
> to be more explicit if necessary. 

Hm. Wouldn't it be just fine if only the server is able to define a 
list of extensions then? It seems to me that all the use-cases of this 
feature require to have a list of extensions defined per server, and 
not per fdw type. This would remove a level of complexity in your 
patch without impacting the feature usability as well. I would 
personally go without it but I am fine to let a committer (Tom?) put a 
final judgement stamp on this matter. Thoughts? 
Simon wanted to be able to define extensions at the wrapper level as well as 
the server level. It’s a nice enough little feature to warrant a small amount 
of added complexity, IMO. I’m going to change the signature, since verbose 
clarity > terse obscurity for my wee brain (only took me a couple weeks to 
forget why that signature was what it wa s).

>> +-- === 
>> +-- clean up 
>> +-- === 
>> Perhaps here you meant dropping the schema and the foreign tables 
>> created previously? 
> 
> I did, but since postgres_fdw doesn't clean up after itself, perhaps 
> just leaving the room messy is the appropriate behaviour? 

That's appropriate when keeping around objects that are aimed to be 
tested with for example pg_upgrade, some regression tests of 
src/test/regress do that on purpose for example. Now it is true that 
an extension regression database will be normally reused by another 
extension just after, and that there is no general rule on this 
I’ll clean up then, I was just being lazy.

P.




-- 
http://postgis.net
http://cleverelephant.ca

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Sat, Sep 26, 2015 at 5:41 AM, Michael Paquier
 wrote:
> On Sat, Sep 26, 2015 at 4:29 AM, Paul Ramsey wrote:
>> On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier wrote:
>> 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);
>> +}
>
> Patch 0001 in this email has a routine called format_type_detailed
> that I think is basically what we need for this stuff:
> http://www.postgresql.org/message-id/CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE=mxvrhefjj51ac...@mail.gmail.com
> Could you look at it?

I'm not sure it helps much. I'd still need a function to turn the
output into a formatted string, but more importantly the big question
for me to avoid breaking regression is: if it's built-in, don't schema
qualitfy it; if it's extended do so. I'm not seeing why ignoring the
typmod in the case of deparsing extended type constants is going to be
a problem? All the old behaviour is untouched in the current patch.

> + * shippable.c
> + *   Non-built-in objects cache management and utilities.
> + *
> + * Is a non-built-in shippable to the remote server? Only if
> + * the object is in an extension declared by the user in the
> + * OPTIONS of the wrapper or the server.
> This is rather unclear. It would be more simple to describe that as
> "Facility to track database objects shippable to a foreign server".

Done

> +extern bool extractExtensionList(char *extensionString,
> +   List **extensionOids);
> What's the point of the boolean status in this new routine? The return
> value of extractExtensionList is never checked, and it would be more
> simple to just return the parsed list as return value, no?

I started changing it, then found out why it is the way it is. During
the options parsing, the list of current extensionOids is passed in,
so that extra ones can be added, since both the wrapper and the server
can be declared with extensionOids. It's also doubling as a flag on
whether the function should bother to try and populate the list, or
just do a sanity check on the options string. I can change the
signature to

extern List* extractExtensionList(char *extensionString, List
*currentExtensionOids, bool populateList);

to be more explicit if necessary.

> -REGRESS = postgres_fdw
> +REGRESS = postgres_fdw shippable
> +EXTRA_INSTALL = contrib/seg
> The order of the tests is important and should be mentioned,
> shippable.sql using the loopback server created by postgres_fdw.

Done

> +-- ===
> +-- clean up
> +-- ===
> Perhaps here you meant dropping the schema and the foreign tables
> created previously?

I did, but since postgres_fdw doesn't clean up after itself, perhaps
just leaving the room messy is the appropriate behaviour?

> +CREATE SCHEMA "SH 1";
> Is there a reason to use double-quoted relations? There is no real
> reason to not use them, this is just to point out that this is not a
> common practice in the other regression tests.

Just following the pattern from postgres_fdw. And since I found things
to be sensitive to full qualification of function names, etc, it
seemed like a good idea to try out odd named tables/schemas since the
pattern was there to follow.

I also changed the extension being tested from 'seg' to 'cube', since
cube had some functions I could test as well as operators.

P.



> --
> Michael
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index d2b98e1..f78fc64 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -1,7 +1,7 @@
 # contrib/postgres_fdw/Makefile
 
 MODULE_big = postgres_fdw
-OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES)
+OBJS = postgres_fdw.o option.o deparse.o connection.o shippable.o $(WIN32RES)
 PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL"
 
 PG_CPPFLAGS = -I$(libpq_srcdir)
@@ -10,7 +10,9 @@ SHLIB_LINK = $(libpq)
 EXTENSION = postgres_fdw
 DATA = postgres_fdw--1.0.sql
 
-REGRESS = postgres_fdw
+# Note: shippable tests depend on postgres_fdw tests setup
+REGRESS = postgres_fdw shippable
+EXTRA_INSTALL = contrib/cube
 
 ifdef USE_PGXS
 PG_CONFIG = pg_config
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 697de60..897ecdb 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -233,6 +233,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;
 
+   /* Access extension metadata from fpinfo on baserel */
+   PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
*)(glob_cxt->foreignrel->fdw_private);
+
/* Need do nothing for empty 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Sat, Sep 26, 2015 at 4:29 AM, Paul Ramsey wrote:
> On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier wrote:
> 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);
> +}

Patch 0001 in this email has a routine called format_type_detailed
that I think is basically what we need for this stuff:
http://www.postgresql.org/message-id/CACACo5Q_UXYwF117LBhjZ3xaMPyrgqnqE=mxvrhefjj51ac...@mail.gmail.com
Could you look at it?

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

Nice. Thanks for the addition of the regression tests. It begins to
have a nice shape.

+ * shippable.c
+ *   Non-built-in objects cache management and utilities.
+ *
+ * Is a non-built-in shippable to the remote server? Only if
+ * the object is in an extension declared by the user in the
+ * OPTIONS of the wrapper or the server.
This is rather unclear. It would be more simple to describe that as
"Facility to track database objects shippable to a foreign server".

+extern bool extractExtensionList(char *extensionString,
+   List **extensionOids);
What's the point of the boolean status in this new routine? The return
value of extractExtensionList is never checked, and it would be more
simple to just return the parsed list as return value, no?

-REGRESS = postgres_fdw
+REGRESS = postgres_fdw shippable
+EXTRA_INSTALL = contrib/seg
The order of the tests is important and should be mentioned,
shippable.sql using the loopback server created by postgres_fdw.

+-- ===
+-- clean up
+-- ===
Perhaps here you meant dropping the schema and the foreign tables
created previously?

+CREATE SCHEMA "SH 1";
Is there a reason to use double-quoted relations? There is no real
reason to not use them, this is just to point out that this is not a
common practice in the other regression tests.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

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

On Thu, Aug 20, 2015 at 6:01 PM, Michael Paquier
 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
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index d2b98e1..63576c4 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -1,7 +1,7 @@
 # contrib/postgres_fdw/Makefile
 
 MODULE_big = postgres_fdw
-OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES)
+OBJS = 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Alvaro Herrera]

Michael Paquier 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 don't know what's going on here, but please look at the patch posted
by Alexander Shulgin in the thread about JSON DDL deparse today; there's
some additional stuff in format_type.c there that is probably useful to
share between these two patches.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[David Fetter]

On Fri, Aug 21, 2015 at 12:55:39PM -0300, Alvaro Herrera wrote:
 Michael Paquier 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 don't know what's going on here, but please look at the patch
 posted by Alexander Shulgin in the thread about JSON DDL deparse
 today; there's some additional stuff in format_type.c there that is
 probably useful to share between these two patches.

Should that stuff be its own stand-alone patch?

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Sat, Aug 22, 2015 at 12:55 AM, Alvaro Herrera alvhe...@2ndquadrant.com
wrote:

 Michael Paquier 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 don't know what's going on here, but please look at the patch posted
 by Alexander Shulgin in the thread about JSON DDL deparse today; there's
 some additional stuff in format_type.c there that is probably useful to
 share between these two patches.


Oh, OK. That's good to know. I'll have a look at it. I think that it may be
possible to extract a single patch usable for both facilities.
-- 
Michael

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Thu, Jul 23, 2015 at 11:48 PM, Paul Ramsey pram...@cleverelephant.ca
wrote:
 On Wed, Jul 22, 2015 at 12:19 PM, Paul Ramsey pram...@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));
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 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

Thanks so much Michael! Let me know when you have further feedback I should 
incorporate.

ATB,
P. 

-- 
http://postgis.net
http://cleverelephant.ca


On July 25, 2015 at 4:52:11 AM, Michael Paquier (michael.paqu...@gmail.com) 
wrote:

On Sat, Jul 25, 2015 at 2:19 AM, Paul Ramsey pram...@cleverelephant.ca wrote: 
 
 On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey pram...@cleverelephant.ca 
 wrote:  
 Here's an updated patch that clears the cache on changes to foreign  
 wrappers and servers.  
  
 Any chance one of you folks could by my official commitfest reviewer?  
 Appreciate all the feedback so far!  
  
 https://commitfest.postgresql.org/6/304/  

That's an interesting topic, I will register as such.  
--  
Michael  

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Sat, Jul 25, 2015 at 2:19 AM, Paul Ramsey pram...@cleverelephant.ca wrote:
 On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey pram...@cleverelephant.ca 
 wrote:
 Here's an updated patch that clears the cache on changes to foreign
 wrappers and servers.

 Any chance one of you folks could by my official commitfest reviewer?
 Appreciate all the feedback so far!

 https://commitfest.postgresql.org/6/304/

That's an interesting topic, I will register as such.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey pram...@cleverelephant.ca wrote:
 Here's an updated patch that clears the cache on changes to foreign
 wrappers and servers.

Any chance one of you folks could by my official commitfest reviewer?
Appreciate all the feedback so far!

https://commitfest.postgresql.org/6/304/

Thanks,

P


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Wed, Jul 22, 2015 at 12:19 PM, Paul Ramsey pram...@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.
In testing it I came across an unrelated issue which could make it
hard for users to manage the options on their wrappers/servers

fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis' );
ALTER SERVER
fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis,seg' );
ERROR:  option extensions provided more than once

Once set, an option seems to be effectively immutable.
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index d2b98e1..3543312 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -1,7 +1,7 @@
 # contrib/postgres_fdw/Makefile
 
 MODULE_big = postgres_fdw
-OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES)
+OBJS = postgres_fdw.o option.o deparse.o connection.o shippable.o $(WIN32RES)
 PGFILEDESC = postgres_fdw - foreign data wrapper for PostgreSQL
 
 PG_CPPFLAGS = -I$(libpq_srcdir)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 81cb2b4..d6fff30 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -229,6 +229,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;
 
+   /* Access extension metadata from fpinfo on baserel */
+   PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
*)(glob_cxt-foreignrel-fdw_private);
+
/* Need do nothing for empty subexpressions */
if (node == NULL)
return true;
@@ -361,7 +364,7 @@ foreign_expr_walker(Node *node,
 * can't be sent to remote because it might 
have incompatible
 * semantics on remote side.
 */
-   if (!is_builtin(fe-funcid))
+   if (!is_builtin(fe-funcid)  
!is_shippable(fe-funcid, fpinfo))
return false;
 
/*
@@ -407,7 +410,7 @@ foreign_expr_walker(Node *node,
 * (If the operator is, surely its underlying 
function is
 * too.)
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_shippable(oe-opno, fpinfo))
return false;
 
/*
@@ -445,7 +448,7 @@ foreign_expr_walker(Node *node,
/*
 * Again, only built-in operators can be sent 
to remote.
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_shippable(oe-opno, fpinfo))
return false;
 
/*
@@ -591,7 +594,7 @@ foreign_expr_walker(Node *node,
 * If result type of given expression is not built-in, it can't be sent 
to
 * remote because it might have incompatible semantics on remote side.
 */
-   if (check_type  !is_builtin(exprType(node)))
+   if (check_type  !is_builtin(exprType(node))  
!is_shippable(exprType(node), fpinfo))
return false;
 
/*
@@ -1404,8 +1407,7 @@ deparseConst(Const *node, deparse_expr_cxt *context)
}
if (needlabel)
appendStringInfo(buf, ::%s,
-
format_type_with_typemod(node-consttype,
-   
  node-consttypmod));
+   format_type_be_qualified(node-consttype));
 }
 
 /*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 7547ec2..9aeaf1a 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -15,6 +15,7 @@
 #include postgres_fdw.h
 
 #include access/reloptions.h
+#include catalog/pg_foreign_data_wrapper.h
 #include catalog/pg_foreign_server.h
 #include catalog/pg_foreign_table.h
 #include catalog/pg_user_mapping.h
@@ -124,6 +125,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 errmsg(%s requires a 
non-negative numeric value,
def-defname)));
}
+   else if (strcmp(def-defname, extensions) == 0)
+   {
+   extractExtensionList(defGetString(def), NULL);
+   }
}
 
PG_RETURN_VOID();
@@ -153,6 +158,9 @@ InitPgFdwOptions(void)
/* updatable is available on both server and table */

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-07-23 07:48:49 -0700, Paul Ramsey wrote:
 fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis' );
 ALTER SERVER
 fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis,seg' );
 ERROR:  option extensions provided more than once
 
 Once set, an option seems to be effectively immutable.

Add SET to the mix and you should be good. The default is ADD, which
explains the problem.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey pram...@cleverelephant.ca wrote:

 In testing it I came across an unrelated issue which could make it
 hard for users to manage the options on their wrappers/servers

 fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis' );
 ALTER SERVER
 fdw=# ALTER SERVER foreign_server OPTIONS ( extensions 'postgis,seg' );
 ERROR:  option extensions provided more than once

 Once set, an option seems to be effectively immutable.

Whoops, I see I just didn't read the fully syntax on providing options, using

ALTER SERVER foreign_server OPTIONS ( SET extensions 'postgis,seg' );

works just fine. Sorry for noise,

P.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Robert Haas]

On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund and...@anarazel.de wrote:
 But I'm not going to complain too loudly if we don't do invalidation.

Not doing invalidation seems silly to me.  But I don't want to bend
Paul too far around the axle, either.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-07-22 14:55:15 -0400, Tom Lane wrote:
 Just to be clear here: the case we are concerned about is, given that
 we have determined that function X is or is not a member of one of the
 extensions marked shippable for a given connection, is it likely that
 that status will change (while the function continues to exist with
 the same OID) during the lifespan of the connection?  If it did change,
 how critical would it be for us to honor the new shippability criterion
 on that connection?  My answer to both is not very.  So I'm not excited
 about expending lots of code or cycles to check for such changes.

It doesn't seem that unlikely that somebody does an ALTER SERVER OPTIONS
SET .. to add an extension to be shippable while connections are already
using the fdw. It'll be confusing if some clients are fast and some
others are really slow.

I think we should at least add invalidations for changing server
options. I think adding support for extension upgrades wouldn't hurt
either. Not particularly excited to ALTER EXTENSION ADD, but we could
add it easy enough.

 If we were trying to cache things across more than a connection lifespan,
 the answer might be different.

I think connection poolers defeat that logic more widely tha nwe
sometimes assume.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Robert Haas robertmh...@gmail.com writes:
 On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund and...@anarazel.de wrote:
 But I'm not going to complain too loudly if we don't do invalidation.

 Not doing invalidation seems silly to me.  But I don't want to bend
 Paul too far around the axle, either.

Just to be clear here: the case we are concerned about is, given that
we have determined that function X is or is not a member of one of the
extensions marked shippable for a given connection, is it likely that
that status will change (while the function continues to exist with
the same OID) during the lifespan of the connection?  If it did change,
how critical would it be for us to honor the new shippability criterion
on that connection?  My answer to both is not very.  So I'm not excited
about expending lots of code or cycles to check for such changes.

If we were trying to cache things across more than a connection lifespan,
the answer might be different.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On July 22, 2015 at 12:15:14 PM, Andres Freund (and...@anarazel.de) wrote:
It doesn't seem that unlikely that somebody does an ALTER SERVER OPTIONS 
SET .. to add an extension to be shippable while connections are already 
using the fdw. It'll be confusing if some clients are fast and some 
others are really slow. 
This seems more likely than anyone mucking around with extension stuff (adding 
new functions (and working with FDW in production at the same time?)) or 
adding/dropping whole extensions (you’ll have more problems than a stale cache, 
whole columns will disappear if you DROP EXTENSION postgis CASCADE), and has 
the added benefit of not needing me to muck into core stuff for my silly 
feature.

I’ll have a look at doing invalidation for the case of changes to the FDW 
wrappers and servers.

P. 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Robert Haas]

On Wed, Jul 22, 2015 at 2:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Robert Haas robertmh...@gmail.com writes:
 On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund and...@anarazel.de wrote:
 But I'm not going to complain too loudly if we don't do invalidation.

 Not doing invalidation seems silly to me.  But I don't want to bend
 Paul too far around the axle, either.

 Just to be clear here: the case we are concerned about is, given that
 we have determined that function X is or is not a member of one of the
 extensions marked shippable for a given connection, is it likely that
 that status will change (while the function continues to exist with
 the same OID) during the lifespan of the connection?  If it did change,
 how critical would it be for us to honor the new shippability criterion
 on that connection?  My answer to both is not very.  So I'm not excited
 about expending lots of code or cycles to check for such changes.

 If we were trying to cache things across more than a connection lifespan,
 the answer might be different.

We've had a few cases like this at EnterpriseDB where we've not done
invalidations in situations like this, and customers have reported
them as bugs.  We've also had cases where PostgreSQL doesn't do this
that have been reported to EnterpriseDB as bugs:

http://www.postgresql.org/message-id/ca+tgmoydf7dkxhktk7u_ynafst47sgk5j8gd8c1jfsiouu1...@mail.gmail.com

If you know what's happening, these kinds of problems are often no big
deal: you just reconnect and it starts working.  The problem is that
customers often DON'T know what is happening, leading to a lot of
frustration and head-banging.  Oh, let me see if reconnecting fixes
it is just not something that tends to occur to people, at least IME.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund and...@anarazel.de wrote:

 +
 + /* We need this relation to scan */
 + depRel = heap_open(DependRelationId, RowExclusiveLock);
 +
 + /* Scan the system dependency table for a all entries this operator */
 + /* depends on, then iterate through and see if one of them */
 + /* is a registered extension */
 + ScanKeyInit(key[0],
 + Anum_pg_depend_objid,
 + BTEqualStrategyNumber, F_OIDEQ,
 + ObjectIdGetDatum(procnumber));
 +
 + scan = systable_beginscan(depRel, DependDependerIndexId, true,
 +   
 GetCatalogSnapshot(depRel-rd_id), nkeys, key);
 +
 + while (HeapTupleIsValid(tup = systable_getnext(scan)))
 + {
 + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup);
 +
 + if ( foundDep-deptype == DEPENDENCY_EXTENSION )
 + {
 + List *extlist = fpinfo-extensions;
 + ListCell *ext;
 +
 + foreach(ext, extlist)
 + {
 + Oid extension_oid = (Oid) lfirst(ext);
 + if ( foundDep-refobjid == extension_oid )
 + {
 + nresults++;
 + }
 + }
 + }
 + if ( nresults  0 ) break;
 + }
 +
 + systable_endscan(scan);
 + relation_close(depRel, RowExclusiveLock);
 +
 + return nresults  0;
 +}

 Phew. That's mighty expensive to do at frequency.

 I guess it'll be more practical to expand this list once and then do a
 binary search on the result for the individual functions

So, right after reading the options in postgresGetForeignRelSize,
expand the extension list into a list of all ops/functions, in a
sorted list, and let that carry through to the deparsing instead? That
would happen once per query, right? But the deparsing also happens
once per query too, yes? Is the difference going to be that big? (I
speak not knowing the overhead of things like systable_beginscan, etc)

Thanks!

P


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-07-21 07:55:17 -0700, Paul Ramsey wrote:
 On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund and...@anarazel.de wrote:
 So, right after reading the options in postgresGetForeignRelSize,
 expand the extension list into a list of all ops/functions, in a
 sorted list, and let that carry through to the deparsing instead?

I'd actually try to make it longer lived, i.e. permanently. And just
deallocate when a catcache callback says it needs to be invalidated;
IIRC there is a relevant cache.

 That
 would happen once per query, right? But the deparsing also happens
 once per query too, yes? Is the difference going to be that big? (I
 speak not knowing the overhead of things like systable_beginscan, etc)

What you have here is an O(nmembers * nexpressions) approach. And each
of those scans is going to do non-neglegible work (an index scan of
pg_depend), that's fairly expensive.

- Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-07-21 17:00:51 +0200, Andres Freund wrote:
 On 2015-07-21 07:55:17 -0700, Paul Ramsey wrote:
  On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund and...@anarazel.de wrote:
  So, right after reading the options in postgresGetForeignRelSize,
  expand the extension list into a list of all ops/functions, in a
  sorted list, and let that carry through to the deparsing instead?
 
 I'd actually try to make it longer lived, i.e. permanently. And just
 deallocate when a catcache callback says it needs to be invalidated;
 IIRC there is a relevant cache.

On second thought I'd not use a binary search but a hash table. If you
choose the right key a single table is enough for the lookup.

If you need references for invalidations you might want to look for
CacheRegisterSyscacheCallback callers. E.g. attoptcache.c


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

Hi,

On 2015-07-21 07:28:22 -0700, Paul Ramsey wrote:
  /*
 @@ -229,6 +236,9 @@ foreign_expr_walker(Node *node,
   Oid collation;
   FDWCollateState state;

 + /* Access extension metadata from fpinfo on baserel */
 + PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
 *)(glob_cxt-foreignrel-fdw_private);
 +
   /* Need do nothing for empty subexpressions */
   if (node == NULL)
   return true;
 @@ -361,7 +371,7 @@ foreign_expr_walker(Node *node,
* can't be sent to remote because it might 
 have incompatible
* semantics on remote side.
*/
 - if (!is_builtin(fe-funcid))
 + if (!is_builtin(fe-funcid)  
 !is_in_extension(fe-funcid, fpinfo))
   return false;

...

  /*
 + * Returns true if given operator/function is part of an extension declared 
 in the
 + * server options.
 + */
 +static bool
 +is_in_extension(Oid procnumber, PgFdwRelationInfo *fpinfo)
 +{
 + static int nkeys = 1;
 + ScanKeyData key[nkeys];
 + HeapTuple tup;
 + Relation depRel;
 + SysScanDesc scan;
 + int nresults = 0;
 +
 + /* Always return false if we don't have any declared extensions */
 + if ( ! fpinfo-extensions )
 + return false;
 +
 + /* We need this relation to scan */
 + depRel = heap_open(DependRelationId, RowExclusiveLock);
 +
 + /* Scan the system dependency table for a all entries this operator */
 + /* depends on, then iterate through and see if one of them */
 + /* is a registered extension */
 + ScanKeyInit(key[0],
 + Anum_pg_depend_objid,
 + BTEqualStrategyNumber, F_OIDEQ,
 + ObjectIdGetDatum(procnumber));
 +
 + scan = systable_beginscan(depRel, DependDependerIndexId, true,
 +   
 GetCatalogSnapshot(depRel-rd_id), nkeys, key);
 +
 + while (HeapTupleIsValid(tup = systable_getnext(scan)))
 + {
 + Form_pg_depend foundDep = (Form_pg_depend) GETSTRUCT(tup);
 +
 + if ( foundDep-deptype == DEPENDENCY_EXTENSION )
 + {
 + List *extlist = fpinfo-extensions;
 + ListCell *ext;
 +
 + foreach(ext, extlist)
 + {
 + Oid extension_oid = (Oid) lfirst(ext);
 + if ( foundDep-refobjid == extension_oid )
 + {
 + nresults++;
 + }
 + }
 + }
 + if ( nresults  0 ) break;
 + }
 +
 + systable_endscan(scan);
 + relation_close(depRel, RowExclusiveLock);
 +
 + return nresults  0;
 +}

Phew. That's mighty expensive to do at frequency.

I guess it'll be more practical to expand this list once and then do a
binary search on the result for the individual functions

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On July 21, 2015 at 11:07:36 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:
I'm inclined to think that it's not really necessary to worry about 
invalidating a per-connection cache of is this function safe to ship 
determinations.
So: yes to a local cache of all forwardable functions/ops, populated in full 
the first time through (does that speak maybe to using a binary search on a 
sorted list instead of a hash, since I only pay the sort price once and am not 
doing any insertions?). And then we just hold it until the connection goes 
away. 

Yes?

P.

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

 
On July 21, 2015 at 8:26:31 AM, Andres Freund 
(and...@anarazel.de(mailto:and...@anarazel.de)) wrote:
 On 2015-07-21 17:00:51 +0200, Andres Freund wrote:
  On 2015-07-21 07:55:17 -0700, Paul Ramsey wrote:
   On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund wrote:
   So, right after reading the options in postgresGetForeignRelSize,
   expand the extension list into a list of all ops/functions, in a
   sorted list, and let that carry through to the deparsing instead?
 
  I'd actually try to make it longer lived, i.e. permanently. And just
  deallocate when a catcache callback says it needs to be invalidated;
  IIRC there is a relevant cache.
  
 On second thought I'd not use a binary search but a hash table. If you
 choose the right key a single table is enough for the lookup.
  
 If you need references for invalidations you might want to look for
 CacheRegisterSyscacheCallback callers. E.g. attoptcache.c

 On 2015-07-21 08:32:34 -0700, Paul Ramsey wrote: 
  Thanks! Reading some of the syscache callback stuff, one thing I’m not 
  sure of is which SysCacheIdentifier(s) I should be registering 
  callbacks against to ensure my list of funcs/procs that reside in 
  particular extensions is kept fresh. I don’t see anything tracking the 
  dependencies there 

 FOREIGNSERVEROID, and a new syscache on pg_extension.oid should suffice 
 I think. pg_foreign_server will be changed upon option changes and 
 pg_extension.oid on extension upgrades. 

 Since dependencies won't change independently of extension versions I 
 don't think we need to care otherwise. There's ALTER EXTENSION ... ADD 
 but I'm rather prepared to ignore that; if that's not ok it's trivial to 
 make it emit an invalidation. 

This hole just keeps getting deeper :) So,

- a HASH in my own code to hold all the functions that I consider “safe to 
forward” (which I’ll derive by reading the contents of the extensions the users 
declare)
- callbacks in my code registered using CacheRegisterSyscacheCallback on 
FOREIGNSERVEROID and __see_below__ that refresh my cache when called
- since there is no syscache for extensions right now, a new syscache entry for 
pg_extension.oid (and I ape things in syscache.h and syscache.c and Magic 
Occurs?)
- which means I can also CacheRegisterSyscacheCallback on the new EXTENSIONOID 
syscache
- and finally change my is_in_extension() to efficiently check my HASH instead 
of querying the system catalog

Folks are going to be OK w/ me dropping in new syscache entries so support my 
niche little feature?

ATB,

P






-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Paul Ramsey pram...@cleverelephant.ca writes:
 Folks are going to be OK w/ me dropping in new syscache entries so support my 
 niche little feature?

No, mainly because it adds overhead without fixing your problem.  It's not
correct to suppose that a syscache on pg_extension would reliably report
anything; consider ALTER EXTENSION ADD/DROP, which does not touch the
pg_extension row.

I'm inclined to think that it's not really necessary to worry about
invalidating a per-connection cache of is this function safe to ship
determinations.  Neither CREATE EXTENSION nor DROP EXTENSION pose any
hazard, nor would ALTER EXTENSION UPGRADE for typical scenarios (which
would only include adding new functions that weren't there before, so
they weren't in your cache anyway).

Anybody who's screwing around with extension membership on-the-fly is
unlikely to expect the system to redetermine ship-ability for active FDW
connections anyway.  If you could do that fully correctly for not a lot of
additional cost, sure; but really anything like this is only going to
take you from 99% to 99.01% coverage of real cases.  Doesn't seem worth
the trouble.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On July 21, 2015 at 11:22:12 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:
 So: yes to a local cache of all forwardable functions/ops, populated in full 
 the first time through (does that speak maybe to using a binary search on a 
 sorted list instead of a hash, since I only pay the sort price once and am 
 not doing any insertions?). And then we just hold it until the connection 
 goes away.   

No, *not* populated first-time-through, because that won't handle any of  
the CREATE, DROP, or UPGRADE cases. It's also doing a lot of work you  
might never need. I was thinking of populate on demand, that is, first  
time you need to know whether function X is shippable, you find that out  
and then cache the answer (whether it be positive or negative).  

Roger that. Off to the races..

P

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Paul Ramsey pram...@cleverelephant.ca writes:
 On July 21, 2015 at 11:07:36 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:
 I'm inclined to think that it's not really necessary to worry about 
 invalidating a per-connection cache of is this function safe to ship 
 determinations.

 So: yes to a local cache of all forwardable functions/ops, populated in full 
 the first time through (does that speak maybe to using a binary search on a 
 sorted list instead of a hash, since I only pay the sort price once and am 
 not doing any insertions?). And then we just hold it until the connection 
 goes away. 

No, *not* populated first-time-through, because that won't handle any of
the CREATE, DROP, or UPGRADE cases.  It's also doing a lot of work you
might never need.  I was thinking of populate on demand, that is, first
time you need to know whether function X is shippable, you find that out
and then cache the answer (whether it be positive or negative).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Andres Freund]

On 2015-07-21 14:07:24 -0400, Tom Lane wrote:
 Paul Ramsey pram...@cleverelephant.ca writes:
  Folks are going to be OK w/ me dropping in new syscache entries so support 
  my niche little feature?
 
 No, mainly because it adds overhead without fixing your problem.

Meh. pg_extension updates are exceedingly rare, and there's a bunch of
code in extension.c that could very well have used a syscache instead of
doing manual scans over the table.

 It's not correct to suppose that a syscache on pg_extension would
 reliably report anything; consider ALTER EXTENSION ADD/DROP, which
 does not touch the pg_extension row.

I'd have just brute-force solved that by forcing a cache inval in that
case.


But I'm not going to complain too loudly if we don't do invalidation.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On Tue, Jul 21, 2015 at 11:29 AM, Paul Ramsey pram...@cleverelephant.ca wrote:
 On July 21, 2015 at 11:22:12 AM, Tom Lane (t...@sss.pgh.pa.us) wrote:

 No, *not* populated first-time-through, because that won't handle any of
 the CREATE, DROP, or UPGRADE cases. It's also doing a lot of work you
 might never need. I was thinking of populate on demand, that is, first
 time you need to know whether function X is shippable, you find that out
 and then cache the answer (whether it be positive or negative).


 Roger that. Off to the races..

Attached, reworked with a local cache. I felt a little dirty sticking
the cache entry right in postgres_fdw.c, so I broke out all my
nonsense into shippable.c.

Thanks!

P
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile
index d2b98e1..3543312 100644
--- a/contrib/postgres_fdw/Makefile
+++ b/contrib/postgres_fdw/Makefile
@@ -1,7 +1,7 @@
 # contrib/postgres_fdw/Makefile
 
 MODULE_big = postgres_fdw
-OBJS = postgres_fdw.o option.o deparse.o connection.o $(WIN32RES)
+OBJS = postgres_fdw.o option.o deparse.o connection.o shippable.o $(WIN32RES)
 PGFILEDESC = postgres_fdw - foreign data wrapper for PostgreSQL
 
 PG_CPPFLAGS = -I$(libpq_srcdir)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 81cb2b4..d6fff30 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -229,6 +229,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;
 
+   /* Access extension metadata from fpinfo on baserel */
+   PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
*)(glob_cxt-foreignrel-fdw_private);
+
/* Need do nothing for empty subexpressions */
if (node == NULL)
return true;
@@ -361,7 +364,7 @@ foreign_expr_walker(Node *node,
 * can't be sent to remote because it might 
have incompatible
 * semantics on remote side.
 */
-   if (!is_builtin(fe-funcid))
+   if (!is_builtin(fe-funcid)  
!is_shippable(fe-funcid, fpinfo))
return false;
 
/*
@@ -407,7 +410,7 @@ foreign_expr_walker(Node *node,
 * (If the operator is, surely its underlying 
function is
 * too.)
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_shippable(oe-opno, fpinfo))
return false;
 
/*
@@ -445,7 +448,7 @@ foreign_expr_walker(Node *node,
/*
 * Again, only built-in operators can be sent 
to remote.
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_shippable(oe-opno, fpinfo))
return false;
 
/*
@@ -591,7 +594,7 @@ foreign_expr_walker(Node *node,
 * If result type of given expression is not built-in, it can't be sent 
to
 * remote because it might have incompatible semantics on remote side.
 */
-   if (check_type  !is_builtin(exprType(node)))
+   if (check_type  !is_builtin(exprType(node))  
!is_shippable(exprType(node), fpinfo))
return false;
 
/*
@@ -1404,8 +1407,7 @@ deparseConst(Const *node, deparse_expr_cxt *context)
}
if (needlabel)
appendStringInfo(buf, ::%s,
-
format_type_with_typemod(node-consttype,
-   
  node-consttypmod));
+   format_type_be_qualified(node-consttype));
 }
 
 /*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 7547ec2..9aeaf1a 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -15,6 +15,7 @@
 #include postgres_fdw.h
 
 #include access/reloptions.h
+#include catalog/pg_foreign_data_wrapper.h
 #include catalog/pg_foreign_server.h
 #include catalog/pg_foreign_table.h
 #include catalog/pg_user_mapping.h
@@ -124,6 +125,10 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
 errmsg(%s requires a 
non-negative numeric value,
def-defname)));
}
+   else if (strcmp(def-defname, extensions) == 0)
+   {
+   extractExtensionList(defGetString(def), NULL);
+   }
}
 
PG_RETURN_VOID();
@@ -153,6 +158,9 @@ InitPgFdwOptions(void)
/* updatable is 

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

Here's a third revision that allows the 'extensions' option on the
wrapper as well, so that supported extensions can be declared once in
one place.

Since the CREATE FOREIGN DATA WRAPPER statement is actually called
inside the CREATE EXTENSION script for postgres_fdw, the way to get
this option is actually to alter the wrapper,

  ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS ( extensions 'seg' );

Right now declaring extensions at different levels is additive, I
didn't add the option to revoke an extension for a particular server
definition (the cube,-postgis entry for example). If that's a
deal-breaker, I can add it too, but it felt like something that could
wait for a user to positively declare I must have that feature!

P.


On Fri, Jul 17, 2015 at 5:58 AM, Paul Ramsey pram...@cleverelephant.ca wrote:

 On July 17, 2015 at 5:57:42 AM, Simon Riggs 
 (si...@2ndquadrant.com(mailto:si...@2ndquadrant.com)) wrote:

 Options already exist on CREATE FOREIGN DATA WRAPPER, so it should be easy 
 to support that.

 I'd rather add it once on the wrapper than be forced to list all the options 
 on every foreign server, unless required to do so.

 Gotcha, that does make sense.

 P.
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 81cb2b4..9c5136e 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -34,11 +34,15 @@
 
 #include postgres_fdw.h
 
+#include access/genam.h
 #include access/heapam.h
 #include access/htup_details.h
 #include access/sysattr.h
 #include access/transam.h
+#include catalog/dependency.h
+#include catalog/indexing.h
 #include catalog/pg_collation.h
+#include catalog/pg_depend.h
 #include catalog/pg_namespace.h
 #include catalog/pg_operator.h
 #include catalog/pg_proc.h
@@ -49,8 +53,10 @@
 #include optimizer/var.h
 #include parser/parsetree.h
 #include utils/builtins.h
+#include utils/fmgroids.h
 #include utils/lsyscache.h
 #include utils/rel.h
+#include utils/snapmgr.h
 #include utils/syscache.h
 
 
@@ -136,6 +142,7 @@ static void printRemoteParam(int paramindex, Oid paramtype, 
int32 paramtypmod,
 deparse_expr_cxt *context);
 static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
   deparse_expr_cxt *context);
+static bool is_in_extension(Oid procid, PgFdwRelationInfo *fpinfo);
 
 
 /*
@@ -229,6 +236,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;
 
+   /* Access extension metadata from fpinfo on baserel */
+   PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
*)(glob_cxt-foreignrel-fdw_private);
+
/* Need do nothing for empty subexpressions */
if (node == NULL)
return true;
@@ -361,7 +371,7 @@ foreign_expr_walker(Node *node,
 * can't be sent to remote because it might 
have incompatible
 * semantics on remote side.
 */
-   if (!is_builtin(fe-funcid))
+   if (!is_builtin(fe-funcid)  
!is_in_extension(fe-funcid, fpinfo))
return false;
 
/*
@@ -407,7 +417,7 @@ foreign_expr_walker(Node *node,
 * (If the operator is, surely its underlying 
function is
 * too.)
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_in_extension(oe-opno, fpinfo))
return false;
 
/*
@@ -445,7 +455,7 @@ foreign_expr_walker(Node *node,
/*
 * Again, only built-in operators can be sent 
to remote.
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
!is_in_extension(oe-opno, fpinfo))
return false;
 
/*
@@ -591,7 +601,7 @@ foreign_expr_walker(Node *node,
 * If result type of given expression is not built-in, it can't be sent 
to
 * remote because it might have incompatible semantics on remote side.
 */
-   if (check_type  !is_builtin(exprType(node)))
+   if (check_type  !is_builtin(exprType(node))  
!is_in_extension(exprType(node), fpinfo))
return false;
 
/*
@@ -669,6 +679,65 @@ is_builtin(Oid oid)
 
 
 /*
+ * Returns true if given operator/function is part of an extension declared in 
the 
+ * server options.
+ */
+static bool
+is_in_extension(Oid procnumber, PgFdwRelationInfo *fpinfo)
+{
+   static int nkeys = 1;
+   ScanKeyData key[nkeys];
+   HeapTuple tup;
+   Relation depRel;
+   SysScanDesc scan;
+   

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

 
On July 17, 2015 at 12:49:04 AM, Simon Riggs 
(si...@2ndquadrant.com(mailto:si...@2ndquadrant.com)) wrote:
 On 17 July 2015 at 01:23, Michael Paquier wrote:
  
   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.
  
 all seems to be a very blunt instrument but is certainly appropriate in 
 some cases  
  
 I see an intermediate setting, giving four categories in total  
  
 0. Nothing, as now  
 1. Extension list option on the Foreign Server
 2. Extension list option on the Foreign Data Wrapper, applies to all Foreign 
 Servers of that type
 3. All extensions allowed

I feel like a lot of knobs are being discussed for maximum configurability, but 
without knowing if people are going to show up with the desire to fiddle them 
:) if marking extensions is not considered a good API, then I’d lean towards 
(a) being able to toggle global fowarding on and off combined with (b) the 
ability to mark individual objects forwardable or not. Which I see, is almost 
what you’ve described.

There’s no facility to add OPTIONS to an EXTENSION right now, so this 
capability seems to be very much server-by-server (adding a FDW-specific 
capability to the EXTENSION mechanism seems like overkill for a niche feature 
addition).

But within the bounds of the SERVER, being able to build combinations of 
allowed/restricted forwarding is certainly powerful,

  CREATE SERVER foo 
    FOREIGN DATA WRAPPER postgres_fdw 
    OPTIONS ( host ‘localhost’, dbname ‘foo’, forward ‘all -postgis’ );

  CREATE SERVER foo 
    FOREIGN DATA WRAPPER postgres_fdw 
    OPTIONS ( host ‘localhost’, dbname ‘foo’, forward ’none +postgis’ );

  CREATE SERVER foo 
    FOREIGN DATA WRAPPER postgres_fdw 
    OPTIONS ( host ‘localhost’, dbname ‘foo’, forward ’none +’ );

Once we get to individual functions or operators it breaks down, since of 
course ‘’ refers to piles of different operators for different types. Their 
descriptions would be unworkably verbose once you have more than a tiny handful.

I’m less concerned with configurability than just the appropriateness of 
forwarding particular functions. In an earlier thread on this topic the problem 
of forwarding arbitrary user-defined PL/PgSQL functions was brought up. In 
passing it was mentioned that maybe VOLATILE functions should *never* be 
forwarded ever. Or, that only IMMUTABLE functions should be *ever* be 
forwarded. Since PostGIS includes a generous mix of all kinds of functions, 
discussing whether that kind of policy is required for any kind of additional 
forwarding would be interesting. Maybe IMMUTABLE/STABLE/VOLATILE doesn’t 
actually capture what it is that makes a function/op FORWARDABLE or not.
 






-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Simon Riggs]

On 17 July 2015 at 13:51, Paul Ramsey pram...@cleverelephant.ca wrote:


 There’s no facility to add OPTIONS to an EXTENSION right now, so this
 capability seems to be very much server-by-server (adding a FDW-specific
 capability to the EXTENSION mechanism seems like overkill for a niche
 feature addition).


Options already exist on CREATE FOREIGN DATA WRAPPER, so it should be easy
to support that.

I'd rather add it once on the wrapper than be forced to list all the
options on every foreign server, unless required to do so.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

On July 17, 2015 at 5:57:42 AM, Simon Riggs 
(si...@2ndquadrant.com(mailto:si...@2ndquadrant.com)) wrote:

 Options already exist on CREATE FOREIGN DATA WRAPPER, so it should be easy to 
 support that.
  
 I'd rather add it once on the wrapper than be forced to list all the options 
 on every foreign server, unless required to do so.  

Gotcha, that does make sense.

P. 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Simon Riggs]

On 17 July 2015 at 01:23, Michael Paquier michael.paqu...@gmail.com wrote:


  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.


all seems to be a very blunt instrument but is certainly appropriate in
some cases

I see an intermediate setting, giving four categories in total

0. Nothing, as now
1. Extension list option on the Foreign Server
2. Extension list option on the Foreign Data Wrapper, applies to all
Foreign Servers of that type
3. All extensions allowed

I would imagine we would need Inclusion and Exclusion on each
e.g. +postgis, -postgis

Cat 2 and 3 would be merged to get the list for the specific server. That
would allow you to a default for all servers of +postgis, yet set a
specific server as -postgis, for example.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
http://www.2ndquadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Amit Langote]

On Thu, Jul 16, 2015 at 11:06 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Amit Langote langote_amit...@lab.ntt.co.jp writes:
 On 2015-07-16 PM 12:43, Tom Lane wrote:
 The basic issue here is how can a user control which functions/operators
 can be sent for remote execution?.  While it's certainly true that
 sometimes you might want function-by-function control of that, Paul's
 point was that extension-level granularity would be extremely convenient
 for PostGIS, and probably for other extensions.

 Perhaps just paranoid but is the extension version number any significant?

 In any scenario for user control of sending functions to the far end, it's
 on the user's head to make sure that he's telling us the truth about which
 functions are compatible between local and remote servers.  That would
 extend to checking cross-version compatibility if he's running different
 versions, too.  We already have risks of that kind with built-in
 functions, really, and I've not heard complaints about it.


Yeah, that's true.

Thanks,
Amit


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Amit Langote langote_amit...@lab.ntt.co.jp writes:
 On 2015-07-16 PM 12:43, Tom Lane wrote:
 The basic issue here is how can a user control which functions/operators
 can be sent for remote execution?.  While it's certainly true that
 sometimes you might want function-by-function control of that, Paul's
 point was that extension-level granularity would be extremely convenient
 for PostGIS, and probably for other extensions.

 Perhaps just paranoid but is the extension version number any significant?

In any scenario for user control of sending functions to the far end, it's
on the user's head to make sure that he's telling us the truth about which
functions are compatible between local and remote servers.  That would
extend to checking cross-version compatibility if he's running different
versions, too.  We already have risks of that kind with built-in
functions, really, and I've not heard complaints about it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

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

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Thu, Jul 16, 2015 at 12:43 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Michael Paquier michael.paqu...@gmail.com writes:
 On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey pram...@cleverelephant.ca 
 wrote:
 Attached is a patch that implements the extension support discussed at
 PgCon this year during the FDW unconference sesssion.

 ...

 Thinking a bit wider, why is this just limited to extensions?

 The basic issue here is how can a user control which functions/operators
 can be sent for remote execution?.

Yep.

 While it's certainly true that
 sometimes you might want function-by-function control of that, Paul's
 point was that extension-level granularity would be extremely convenient
 for PostGIS, and probably for other extensions.  I don't see anything
 wrong with that --- and I don't think that we should insist that Paul's
 patch implement both cases.  Somebody else who really needs
 function-by-function control can do the dogwork of figuring out a
 reasonable API for that.

Well, you could for example pass a JSON string (that's fashionable
these days) that sets up a list of authorized objects per category
instead, like:
authorized_objects = {functions:[foo_oid,foo2_oid],
operators:[ope1_oid,ope2_oid]}

 Disclaimer 1: Paul and I discussed this back at PGCon, and I encouraged
 him to send in his patch.

 Disclaimer 2: I haven't read the patch and don't mean to vouch for any
 implementation details.  But the functional spec of allow remote
 execution of functions belonging to named extensions seems sane to me.

Well, I am just questioning the extensibility of the proposed
interface, not saying that this is a bad thing :)
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Tom Lane]

Michael Paquier michael.paqu...@gmail.com writes:
 On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey pram...@cleverelephant.ca 
 wrote:
 Attached is a patch that implements the extension support discussed at
 PgCon this year during the FDW unconference sesssion.

...

 Thinking a bit wider, why is this just limited to extensions?

The basic issue here is how can a user control which functions/operators
can be sent for remote execution?.  While it's certainly true that
sometimes you might want function-by-function control of that, Paul's
point was that extension-level granularity would be extremely convenient
for PostGIS, and probably for other extensions.  I don't see anything
wrong with that --- and I don't think that we should insist that Paul's
patch implement both cases.  Somebody else who really needs
function-by-function control can do the dogwork of figuring out a
reasonable API for that.

Disclaimer 1: Paul and I discussed this back at PGCon, and I encouraged
him to send in his patch.

Disclaimer 2: I haven't read the patch and don't mean to vouch for any
implementation details.  But the functional spec of allow remote
execution of functions belonging to named extensions seems sane to me.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Amit Langote]

On 2015-07-16 PM 12:43, Tom Lane wrote:
 
 The basic issue here is how can a user control which functions/operators
 can be sent for remote execution?.  While it's certainly true that
 sometimes you might want function-by-function control of that, Paul's
 point was that extension-level granularity would be extremely convenient
 for PostGIS, and probably for other extensions.

Perhaps just paranoid but is the extension version number any significant?
I guess that if a function name is matched locally and declared safe to
send but doesn't really exist on the remote end or does exist but has
different behavior, then that can't be expected to work or work correctly.
But it seems difficult to incorporate the version number into chosen
approach of matching functions anyway.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] postgres_fdw extension support

[Michael Paquier]

On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey pram...@cleverelephant.ca wrote:
 Attached is a patch that implements the extension support discussed at
 PgCon this year during the FDW unconference sesssion. Highlights:

 * Pass extension operators and functions to the foreign server
 * Only send ops/funcs if the foreign server is declared to support the
 relevant extension, for example:

 CREATE SERVER foreign_server
 FOREIGN DATA WRAPPER postgres_fdw
 OPTIONS (host '127.0.0.1', port '5432', dbname 'my_db',
 extensions 'cube, seg');

 Github branch is here:
   https://github.com/pramsey/postgres/tree/fdw-extension-suppport

 Synthetic pull request for easy browsing/commenting is here:
   https://github.com/pramsey/postgres/pull/1


+
 /*
  * Returns true if given expr is safe to evaluate on the foreign server.
  */
@@ -177,7 +185,7 @@ is_foreign_expr(PlannerInfo *root,
 {
 foreign_glob_cxt glob_cxt;
 foreign_loc_cxt loc_cxt;
-
+
 /*
  * Check that the expression consists of nodes that are safe to execute
  * remotely.
@@ -207,6 +215,8 @@ is_foreign_expr(PlannerInfo *root,
 return true;
 }

+
+

This patch includes some diff noise, it would be better to remove that.

-if (!is_builtin(fe-funcid))
+if (!is_builtin(fe-funcid) 
(!is_in_extension(fe-funcid, fpinfo)))
Extra parenthesis are not needed.

+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

+/* PostGIS metadata */
+List*extensions;
+booluse_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?
(And you could rely on the fact that postgis_oid is InvalidOid to
determine if it is defined or not btw). I have a hard time
understanding why this refers to PostGIS as well as a postgres_fdw
feature.

 appendStringInfo(buf, ::%s,
- format_type_with_typemod(node-consttype,
-  node-consttypmod));
+ format_type_be_qualified(node-consttype));
What's the reason for this change?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH] postgres_fdw extension support

[Paul Ramsey]

Hi all,

Attached is a patch that implements the extension support discussed at
PgCon this year during the FDW unconference sesssion. Highlights:

* Pass extension operators and functions to the foreign server
* Only send ops/funcs if the foreign server is declared to support the
relevant extension, for example:

CREATE SERVER foreign_server
FOREIGN DATA WRAPPER postgres_fdw
OPTIONS (host '127.0.0.1', port '5432', dbname 'my_db',
extensions 'cube, seg');

Github branch is here:
  https://github.com/pramsey/postgres/tree/fdw-extension-suppport

Synthetic pull request for easy browsing/commenting is here:
  https://github.com/pramsey/postgres/pull/1

Thanks!

Paul
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 81cb2b4..bbe3c9d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -34,11 +34,15 @@
 
 #include postgres_fdw.h
 
+#include access/genam.h
 #include access/heapam.h
 #include access/htup_details.h
 #include access/sysattr.h
 #include access/transam.h
+#include catalog/dependency.h
+#include catalog/indexing.h
 #include catalog/pg_collation.h
+#include catalog/pg_depend.h
 #include catalog/pg_namespace.h
 #include catalog/pg_operator.h
 #include catalog/pg_proc.h
@@ -49,8 +53,10 @@
 #include optimizer/var.h
 #include parser/parsetree.h
 #include utils/builtins.h
+#include utils/fmgroids.h
 #include utils/lsyscache.h
 #include utils/rel.h
+#include utils/snapmgr.h
 #include utils/syscache.h
 
 
@@ -136,6 +142,7 @@ static void printRemoteParam(int paramindex, Oid paramtype, 
int32 paramtypmod,
 deparse_expr_cxt *context);
 static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
   deparse_expr_cxt *context);
+static bool is_in_extension(Oid procid, PgFdwRelationInfo *fpinfo);
 
 
 /*
@@ -167,6 +174,7 @@ classifyConditions(PlannerInfo *root,
}
 }
 
+
 /*
  * Returns true if given expr is safe to evaluate on the foreign server.
  */
@@ -177,7 +185,7 @@ is_foreign_expr(PlannerInfo *root,
 {
foreign_glob_cxt glob_cxt;
foreign_loc_cxt loc_cxt;
-
+   
/*
 * Check that the expression consists of nodes that are safe to execute
 * remotely.
@@ -207,6 +215,8 @@ is_foreign_expr(PlannerInfo *root,
return true;
 }
 
+
+
 /*
  * Check if expression is safe to execute remotely, and return true if so.
  *
@@ -229,6 +239,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;
 
+   /* Access extension metadata from fpinfo on baserel */
+   PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo 
*)(glob_cxt-foreignrel-fdw_private);
+
/* Need do nothing for empty subexpressions */
if (node == NULL)
return true;
@@ -361,7 +374,7 @@ foreign_expr_walker(Node *node,
 * can't be sent to remote because it might 
have incompatible
 * semantics on remote side.
 */
-   if (!is_builtin(fe-funcid))
+   if (!is_builtin(fe-funcid)  
(!is_in_extension(fe-funcid, fpinfo)))
return false;
 
/*
@@ -407,7 +420,7 @@ foreign_expr_walker(Node *node,
 * (If the operator is, surely its underlying 
function is
 * too.)
 */
-   if (!is_builtin(oe-opno))
+   if ( (!is_builtin(oe-opno))  
(!is_in_extension(oe-opno, fpinfo)) )
return false;
 
/*
@@ -445,7 +458,7 @@ foreign_expr_walker(Node *node,
/*
 * Again, only built-in operators can be sent 
to remote.
 */
-   if (!is_builtin(oe-opno))
+   if (!is_builtin(oe-opno)  
(!is_in_extension(oe-opno, fpinfo)))
return false;
 
/*
@@ -591,7 +604,7 @@ foreign_expr_walker(Node *node,
 * If result type of given expression is not built-in, it can't be sent 
to
 * remote because it might have incompatible semantics on remote side.
 */
-   if (check_type  !is_builtin(exprType(node)))
+   if (check_type  !is_builtin(exprType(node))  
(!is_in_extension(exprType(node), fpinfo)) )
return false;
 
/*
@@ -643,6 +656,8 @@ foreign_expr_walker(Node *node,
return true;
 }
 
+
+
 /*
  * Return true if given object is one of PostgreSQL's built-in objects.
  *
@@ -669,6 +684,67 @@ is_builtin(Oid oid)
 
 
 /*
+ * Returns true if given operator/function is part of an extension declared in 
the