[PATCH] postgres_fdw extension support

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

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

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

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

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

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

On 2015-07-16 PM 12:43, Tom Lane wrote:

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

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

Thanks,
Amit

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

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

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

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

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

Michael, thanks so much for the review!

On July 15, 2015 at 7:35:11 PM, Michael Paquier (michael.paquier@gmail.com) wrote:
This patch includes some diff noise, it would be better to remove that. 
Done.

- if (!is_builtin(fe->funcid)) 
+ if (!is_builtin(fe->funcid) && 
(!is_in_extension(fe->funcid, fpinfo))) 
Extra parenthesis are not needed. 
OK, will remove.

+ if ( (!is_builtin(oe->opno)) && 
(!is_in_extension(oe->opno, fpinfo)) ) 
... And this does not respect the project code format. See here for 
more details for example: 
http://www.postgresql.org/docs/devel/static/source.html 
I’m sorry, that link doesn’t clarify for me what’s stylistically wrong here (it’s almost all about error messages), could you help (is it the padding around the conditionals? removed that)

+ /* PostGIS metadata */ 
+ List *extensions; 
+ bool use_postgis; 
+ Oid postgis_oid; 
This addition in PgFdwRelationInfo is surprising. What the point of 
keeping use_postgis and postgres_oid that are actually used nowhere? 
Whoops, a couple old pieces from my proof-of-concept survived the conversion to a generic features. Removed.

appendStringInfo(buf, "::%s", 
- format_type_with_typemod(node->consttype, 
- node->consttypmod)); 
+ format_type_be_qualified(node->consttype)); 
What's the reason for this change? 
Good question. As I recall, the very sparse search path the FDW connection makes can sometimes leave remote function failing to find other functions they need, so we want to force the calls to be schema-qualified. Unfortunately there’s no perfect public call for that, ideally it would be return format_type_internal(type_oid, typemod, true, false, true), but there’s no function like that, so I settled for format_type_be_qualified(), which forces qualification at the expense of ignoring the typmod.

Thinking a bit wider, why is this just limited to extensions? You may 
have as well other objects defined locally and remotely like functions 
or operators that are not defined in extensions, but as normal 
objects. Hence I think that what you are looking for is not this 
option, but a boolean option enforcing the behavior of code paths 
using is_builtin() in foreign_expr_walker such as the sanity checks on 
existing objects are not limited to FirstBootstrapObjectId but to 
other objects in the catalogs. 
Well, as I see it there’s three broad categories of behavior available:

1- Forward nothing non-built-in (current behavior)

2- Use options to forward only specified non-built-in things (either in function chunks (extensions, as in this patch) or one-by-one (mark your desired functions / ops)

3- Forward everything if a “forward everything” option is set

I hadn’t actually considered the possibility of option 3, but for my purposes it would work just as well, with the added efficiency bonus of not having to check whether particular funcs/ops are inside declared extensions. Both the current state of FDW and the patch I’m providing expect a *bit* of smarts on the part of users, to make sure their remote/local environments are in sync (in particular versions of pgsql and of extensions). Option 3 just ups the ante on that requirement. I’d be fine w/ this, makes the patch very simple and fast.

For option 2, marking things one at a time really isn’t practical for a package like PostGIS, with several hundred functions and operators. Using the larger block of “extension” makes more sense. I think in general, expecting users to itemize every func/op they want to forward, particular if they just want an extension to “work” over FDW is too big an expectation. That’s not to minimize the utility of being able to mark individual functions/ops for forwarding, but I think that’s a separate use case that doesn’t eliminate the need for extension-level forwarding.

Thanks again for the review!

P.

That's a risky option because it could 
lead to inconsistencies among objects, so obviously the default is 
false but by documenting correctly the risks of using this option we 
may be able to get something integrated (aka be sure that each object 
is defined consistently across the servers queried or you'll have 
surprising results!). In short, it seems to me that you are taking the 
wrong approach. 
-- 

On Fri, Jul 17, 2015 at 1:08 AM, Paul Ramsey wrote:

+ if ( (!is_builtin(oe->opno)) &&
(!is_in_extension(oe->opno, fpinfo)) )
... And this does not respect the project code format. See here for
more details for example:
http://www.postgresql.org/docs/devel/static/source.html

I’m sorry, that link doesn’t clarify for me what’s stylistically wrong here
(it’s almost all about error messages), could you help (is it the padding
around the conditionals? removed that)

Two things:
1) Spaces between parenthesis at the beginning and the end of he expression
2) Parenthesis around is_in_extension are not that much necessary.

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

Good question. As I recall, the very sparse search path the FDW connection
makes can sometimes leave remote function failing to find other functions
they need, so we want to force the calls to be schema-qualified.
Unfortunately there’s no perfect public call for that, ideally it would be
return format_type_internal(type_oid, typemod, true, false, true), but
there’s no function like that, so I settled for format_type_be_qualified(),
which forces qualification at the expense of ignoring the typmod.

Hm. I don't have my mind wrapped around that now, but this needs to be
checked with data types like char or varchar. It may matter.

Thinking a bit wider, why is this just limited to extensions? You may
have as well other objects defined locally and remotely like functions
or operators that are not defined in extensions, but as normal
objects. Hence I think that what you are looking for is not this
option, but a boolean option enforcing the behavior of code paths
using is_builtin() in foreign_expr_walker such as the sanity checks on
existing objects are not limited to FirstBootstrapObjectId but to
other objects in the catalogs.

Well, as I see it there’s three broad categories of behavior available:

1- Forward nothing non-built-in (current behavior)
2- Use options to forward only specified non-built-in things (either in
function chunks (extensions, as in this patch) or one-by-one (mark your
desired functions / ops)
3- Forward everything if a “forward everything” option is set

Then what you are describing here is a parameter able to do a switch
among this selection:
- nothing, which is to check on built-in stuff
- extension list.
- all.

I hadn’t actually considered the possibility of option 3, but for my
purposes it would work just as well, with the added efficiency bonus of not
having to check whether particular funcs/ops are inside declared extensions.
Both the current state of FDW and the patch I’m providing expect a *bit* of
smarts on the part of users, to make sure their remote/local environments
are in sync (in particular versions of pgsql and of extensions). Option 3
just ups the ante on that requirement. I’d be fine w/ this, makes the patch
very simple and fast.

Yeah, perhaps too simple though :) You may want something that is more
advanced. For example when using a set of objects you can decide which
want you want to pushdown and which one you want to keep as evaluated
locally.

For option 2, marking things one at a time really isn’t practical for a
package like PostGIS, with several hundred functions and operators. Using
the larger block of “extension” makes more sense. I think in general,
expecting users to itemize every func/op they want to forward, particular if
they just want an extension to “work” over FDW is too big an expectation.
That’s not to minimize the utility of being able to mark individual
functions/ops for forwarding, but I think that’s a separate use case that
doesn’t eliminate the need for extension-level forwarding.

OK, that's good to know. Perhaps then that using a parameter with an
extension list is a good balance. Now would there be practical cases
where it is actually useful to have an extension list? For example,
let's say that there are two extensions installed: foo1 and foo2. Do
you think (or know) if some users would be willing to include only the
objects of foo1, but include those of foo2? Also, could it be possible
to consider an exclude list? Like ignoring all the objects in the
given extension list and allow the rest.
I am just trying to consider all the possibilities here.

Thanks again for the review!

Good to see that you added in the CF app:
https://commitfest.postgresql.org/6/304/

Regards,
--
Michael

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

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

Well, as I see it there’s three broad categories of behavior available:

1- Forward nothing non-built-in (current behavior)
2- Use options to forward only specified non-built-in things (either in
function chunks (extensions, as in this patch) or one-by-one (mark your
desired functions / ops)
3- Forward everything if a “forward everything” option is set

Then what you are describing here is a parameter able to do a switch
among this selection:
- nothing, which is to check on built-in stuff
- extension list.
- all.

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

I see an intermediate setting, giving four categories in total

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

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

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

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

 
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 show up with the desire to fiddle them :) if marking extensions is not considered a good API, then I’d lean towards (a) being able to toggle global fowarding on and off combined with (b) the ability to mark individual objects forwardable or not. Which I see, is almost what you’ve described.

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

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

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

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

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

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

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

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

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

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

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

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

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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 to do so.

Gotcha, that does make sense.

P. 

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

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.

Hi,

On 2015-07-21 07:28:22 -0700, Paul Ramsey wrote:

/*
@@ -229,6 +236,9 @@ foreign_expr_walker(Node *node,
Oid collation;
FDWCollateState state;

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

...

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

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

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

Greetings,

Andres Freund

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

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

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

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

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

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

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

 
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 contents of the extensions the users declare)
- callbacks in my code registered using CacheRegisterSyscacheCallback on FOREIGNSERVEROID and __see_below__ that refresh my cache when called
- since there is no syscache for extensions right now, a new syscache entry for pg_extension.oid (and I ape things in syscache.h and syscache.c and Magic Occurs?)
- which means I can also CacheRegisterSyscacheCallback on the new EXTENSIONOID syscache
- and finally change my is_in_extension() to efficiently check my HASH instead of querying the system catalog

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

ATB,

P

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

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

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