Thread: [PATCH] postgres_fdw extension support

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

On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey <pramsey@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;
+    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?
(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

Michael Paquier <michael.paquier@gmail.com> writes:
> On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey <pramsey@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

On Thu, Jul 16, 2015 at 12:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Thu, Jul 16, 2015 at 3:43 AM, Paul Ramsey <pramsey@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

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

Amit Langote <Langote_Amit_f8@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

On Thu, Jul 16, 2015 at 11:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <Langote_Amit_f8@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

Done.

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

 
On July 17, 2015 at 12:49:04 AM, Simon Riggs (simon@2ndquadrant.com(mailto:simon@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
showup with the desire to fiddle them :) if marking extensions is not considered a good API, then I’d lean towards (a)
beingable 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
(addinga 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
operatorsfor 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
threadon this topic the problem of forwarding arbitrary user-defined PL/PgSQL functions was brought up. In passing it
wasmentioned 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
policyis required for any kind of additional forwarding would be interesting. Maybe IMMUTABLE/STABLE/VOLATILE doesn’t
actuallycapture what it is that makes a function/op FORWARDABLE or not. 
 

On July 17, 2015 at 5:57:42 AM, Simon Riggs (simon@2ndquadrant.com(mailto:simon@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
todo so.   

Gotcha, that does make sense.

P. 

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 <pramsey@cleverelephant.ca> wrote:
>
> On July 17, 2015 at 5:57:42 AM, Simon Riggs (simon@2ndquadrant.com(mailto:simon@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
requiredto do so. 
>
> Gotcha, that does make sense.
>
> P.

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

On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund <andres@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

On 2015-07-21 07:55:17 -0700, Paul Ramsey wrote:
> On Tue, Jul 21, 2015 at 7:45 AM, Andres Freund <andres@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

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 <andres@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

 
On July 21, 2015 at 8:26:31 AM, Andres Freund (andres@anarazel.de(mailto:andres@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
contentsof the extensions the users declare) 
- callbacks in my code registered using CacheRegisterSyscacheCallback on FOREIGNSERVEROID and __see_below__ that
refreshmy 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.hand 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

Paul Ramsey <pramsey@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

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.

Paul Ramsey <pramsey@cleverelephant.ca> writes:
> On July 21, 2015 at 11:07:36 AM, Tom Lane (tgl@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
maybeto using a binary search on a sorted list instead of a hash, since I only pay the sort price once and am not doing
anyinsertions?). 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

On 2015-07-21 14:07:24 -0400, Tom Lane wrote:
> Paul Ramsey <pramsey@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

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

On Tue, Jul 21, 2015 at 11:29 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> On July 21, 2015 at 11:22:12 AM, Tom Lane (tgl@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

On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund <andres@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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund <andres@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

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

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. 

On Wed, Jul 22, 2015 at 2:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Jul 21, 2015 at 2:27 PM, Andres Freund <andres@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_YnAfSt47SgK5J8gD8C1JfSiouU194g@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

On Wed, Jul 22, 2015 at 12:19 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>
> I’ll have a look at doing invalidation for the case of changes to the FDW
> wrappers and servers.

Here's an updated patch that clears the cache on changes to foreign
wrappers and servers.
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.

On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey <pramsey@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.

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

On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey <pramsey@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

On Sat, Jul 25, 2015 at 2:19 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey <pramsey@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

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

On Sat, Jul 25, 2015 at 2:19 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> On Thu, Jul 23, 2015 at 7:48 AM, Paul Ramsey <pramsey@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.com

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

<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Sat, Aug 22, 2015 at 12:55 AM, Alvaro
Herrera<span dir="ltr"><<a href="mailto:alvherre@2ndquadrant.com"
target="_blank">alvherre@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"><span class="">Michael Paquier wrote:<br /><br /> >         if
(needlabel)<br/> >                 appendStringInfo(buf, "::%s",<br /> > -<br /> >
format_type_with_typemod(node->consttype,<br/> > -<br /> > node->consttypmod));<br /> > +<br /> >
format_type_be_qualified(node->consttype));<br/> > Pondering more about this one, I think that we are going to
needa new<br /> > routine in format_type.c to be able to call format_type_internal as<br /> >
format_type_internal(type_oid,typemod, true/false, false, true). If<br /> > typemod is -1, then typemod_given (the
thirdargument) is false, otherwise<br /> > typemod_given is true. That's close to what the C function format_type
at<br/> > the SQL level can do except that we want it to be qualified. Regression<br /> > tests will need an
updateas well.<br /><br /></span>I don't know what's going on here, but please look at the patch posted<br /> by
AlexanderShulgin in the thread about JSON DDL deparse today; there's<br /> some additional stuff in format_type.c there
thatis probably useful to<br /> share between these two patches.</blockquote></div><br /></div><div
class="gmail_extra">Oh,OK. That's good to know. I'll have a look at it. I think that it may be possible to extract a
singlepatch usable for both facilities.<br />-- <br /><div class="gmail_signature">Michael<br /></div></div></div> 

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

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

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

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

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

Comments fixed.

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

Done

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

Done

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

Done.

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

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

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

Done.

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

Done.

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

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

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

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

Thanks so much!

P.

> Regards,
> --
> Michael

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=mXvRhEfjJ51aCfwQ@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

On Sat, Sep 26, 2015 at 5:41 AM, Michael Paquier
<michael.paquier@gmail.com> 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=mXvRhEfjJ51aCfwQ@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

On Mon, Sep 28, 2015 at 10:47 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
> On Sat, Sep 26, 2015 at 5:41 AM, Michael Paquier
> <michael.paquier@gmail.com> 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=mXvRhEfjJ51aCfwQ@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

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

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

Paul Ramsey <pramsey@cleverelephant.ca> 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

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. 

On Thu, Oct 1, 2015 at 4:33 AM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
>
>
> On September 30, 2015 at 7:06:58 AM, Tom Lane (tgl@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

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.

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

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. */
> +    QualCost    local_conds_cost;
> +    Selectivity local_conds_sel;
> +
> +    /* Estimated size and cost for a scan with baserestrictinfo quals. */
> +    double        rows;
> +    int            width;
> +    Cost        startup_cost;
> +    Cost        total_cost;
> +
> +    /* Options extracted from catalogs. */
> +    bool        use_remote_estimate;
> +    Cost        fdw_startup_cost;
> +    Cost        fdw_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(&status, ShippableCacheHash);
> +    while ((entry = (ShippableCacheEntry *) hash_seq_search(&status)) != NULL)
> +    {
> +        if (hash_search(ShippableCacheHash,
> +                        (void *) &entry->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(&key[0],
> +                Anum_pg_depend_objid,
> +                BTEqualStrategyNumber, F_OIDEQ,
> +                ObjectIdGetDatum(objnumber));
> +
> +    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_member_oid(extension_list, foundDep->refobjid))
> +        {
> +            is_shippable = true;
> +            break;
> +        }
> +    }

Hm.

> +/*
> + * 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, List *extension_list)
> +{
> +    ShippableCacheKey key;
> +    ShippableCacheEntry *entry;
> +
> +    /* Always return false if we don't have any declared extensions */
> +    if (extension_list == NIL)
> +        return false;

I again dislike declared here ;)

> +    /* Find existing cache, if any. */
> +    if (!ShippableCacheHash)
> +        InitializeShippableCache();
> +
> +    /* Zero out the key */
> +    memset(&key, 0, sizeof(key));
> +
> +    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?

> +    entry = (ShippableCacheEntry *)
> +                 hash_search(ShippableCacheHash,
> +                    (void *) &key,
> +                    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, extension_list);
> +
> +        entry = (ShippableCacheEntry *)
> +                     hash_search(ShippableCacheHash,
> +                        (void *) &key,
> +                        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.

Greetings,

Andres Freund

But “throw up” is so evocative :) fixed.

On Sun, Oct 4, 2015 at 11:40 AM, Paul Ramsey <pramsey@cleverelephant.ca> 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

 
On October 4, 2015 at 9:56:10 PM, Michael Paquier (michael.paquier@gmail.com(mailto:michael.paquier@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
buttingup against the principle of language usage consistency here. Can remove, of course. 

P 


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

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 *) &key, 
> > > +     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, &found);

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

On Tue, Oct 6, 2015 at 5:32 AM, Andres Freund <andres@anarazel.de> 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.

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(&key, 0, sizeof(key));
> +
> +    key.objid = objnumber;
> +    key.classid = classnumber;
> +
> +    entry = (ShippableCacheEntry *)
> +                 hash_search(ShippableCacheHash,
> +                    (void *) &key,
> +                    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 *) &key,
> +                        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

On October 6, 2015 at 6:32:36 AM, Andres Freund (andres@anarazel.de(mailto:andres@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(&key, 0, sizeof(key));
> > +
> > + key.objid = objnumber;
> > + key.classid = classnumber;
> > +
> > + entry = (ShippableCacheEntry *)
> > + hash_search(ShippableCacheHash,
> > + (void *) &key,
> > + 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 *) &key,
> > + 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
testthat ensures they are shippable for this *particular* server (which only happens in the lookup stage). So yeah, the
cachewill be wrong in cases where different servers have different extension opti ons. 

Thanks, 
P.


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

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

On Tue, Oct 6, 2015 at 6:55 AM, Andres Freund <andres@anarazel.de> 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

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

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

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

Done,
P

Paul Ramsey <pramsey@cleverelephant.ca> 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

On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Paul Ramsey <pramsey@cleverelephant.ca> 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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Paul Ramsey <pramsey@cleverelephant.ca> 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

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Nov 3, 2015 at 2:57 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Paul Ramsey <pramsey@cleverelephant.ca> 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

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

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

Michael Paquier <michael.paquier@gmail.com> 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

On Wed, Nov 4, 2015 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> 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