Обсуждение: Is there a way around function search_path killing SQL function inlining?

I think the answer to this question is NO, but thought I'd ask. 

A lot of folks in PostGIS land are suffering from restore issues,
materialized view issues etc. because we have functions such as

ST_Intersects

Which does _ST_Intersects  AND && 

Since _ST_Intersects is not schema qualified, during database restore (which
sets the schema to the table or view schema), materialized views that depend
on this do not come back.
It's also a serious issue with raster, though that one can be fixed by
setting search_path since the issue there doesn't use SQL inlining.

So I had this bright idea of setting the search_path of the functions to
where PostGIS is installed.

https://trac.osgeo.org/postgis/ticket/3490

To my disappointment, I noticed our spatial indexes no longer worked if I do
this since they rely on SQL inlining.

Schema qualifying our function calls is not an option at this time since

1) People install postgis in different schemas so we'd have to force them to
install in the same schema which I think will break a lot of 3rd party apps.
2) It's a lot of functions to hand touch.

Any suggestions are welcome.  Any other issues I should be aware of with 

ALTER FUNCTION .. set search_path..

Only other I noticed is it seems to be ignored in CREATE EXTENSION script
(at least when using dynamic execute).  I put it in and it seems to be
entirely ignored.

Thanks,
Regina

On Fri, Mar 4, 2016 at 9:29 PM, Regina Obe <lr@pcorp.us> wrote:
> I think the answer to this question is NO, but thought I'd ask.
>
> A lot of folks in PostGIS land are suffering from restore issues,
> materialized view issues etc. because we have functions such as
>
> ST_Intersects
>
> Which does _ST_Intersects  AND &&
>
> Since _ST_Intersects is not schema qualified, during database restore (which
> sets the schema to the table or view schema), materialized views that depend
> on this do not come back.

Could you provide a self-contained, reproducible test case that
illustrates this problem?  Ideally, one that doesn't involve
installing PostGIS?

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

>> On Fri, Mar 4, 2016 at 9:29 PM, Regina Obe <lr@pcorp.us>> wrote:
>> I think the answer to this question is NO, but thought I'd ask.
>>
>> A lot of folks in PostGIS land are suffering from restore issues,
>> materialized view issues etc. because we have functions such as
>>
>> ST_Intersects
>>
>> Which does _ST_Intersects  AND &&
>>
>> Since _ST_Intersects is not schema qualified, during database restore
>> (which sets the schema to the table or view schema), materialized
>> views that depend on this do not come back.

> Could you provide a self-contained, reproducible test case that illustrates this problem?  Ideally, one that doesn't
involveinstalling PostGIS? 

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

Here is a script just involving the built in geometric types that has the same issue:


-- script starts here --
CREATE schema funcs;

set search_path=public,funcs;
CREATE OR REPLACE FUNCTION funcs._helper(box, box) RETURNS float8 AS
$$ SELECT box_distance($1,$2);
$$
language 'sql' IMMUTABLE STRICT;


CREATE OR REPLACE FUNCTION funcs.no_inline(box,box) RETURNS boolean AS
$$SELECT $1 && $2 AND _helper($1,$2) = 0;
$$
language 'sql' IMMUTABLE;

--doing this kills inlining
ALTER FUNCTION funcs.no_inline(box, box) SET search_path=funcs;


--this one doesn't have search_path set so inlining works
CREATE OR REPLACE FUNCTION funcs.inline(box,box) RETURNS boolean AS
$$SELECT $1 && $2 AND _helper($1,$2) = 0;
$$
language 'sql' IMMUTABLE;


CREATE TABLE bag_boxes(id serial primary key, geom box);
CREATE INDEX idx_bag_boxes_geom ON bag_boxes USING gist(geom);

INSERT INTO bag_boxes(geom)
SELECT ('((' || i::text || ',' || j::text || '), (' || k::text || ', ' || l::text || '))')::box
FROM generate_series(1,10) i , generate_series(11,20) j, generate_series(5,10) k, generate_series(10, 15) l ;


SELECT b1.id, b2.id As id2
FROM bag_boxes AS b1 INNER JOIN bag_boxes As b2 ON no_inline(b1.geom, b2.geom);

-- plan looks like this -- PostgreSQL 9.5.1, compiled by Visual C++ build 1800, 32-bit
/** Nested Loop  (cost=0.00..3402141.00 rows=4320000 width=8) Join Filter: no_inline(b1.geom, b2.geom) ->  Seq Scan on
bag_boxesb1  (cost=0.00..66.00 rows=3600 width=36) ->  Materialize  (cost=0.00..84.00 rows=3600 width=36)       ->  Seq
Scanon bag_boxes b2  (cost=0.00..66.00 rows=3600 width=36) **/ 



SELECT b1.id, b2.id As id2
FROM bag_boxes AS b1 INNER JOIN bag_boxes As b2 ON inline(b1.geom, b2.geom);


-- plan looks like this PostgreSQL 9.5.1, compiled by Visual C++ build 1800, 32-bit
/** Nested Loop  (cost=0.15..2359.00 rows=324 width=8) ->  Seq Scan on bag_boxes b1  (cost=0.00..66.00 rows=3600
width=36)->  Index Scan using idx_bag_boxes_geom on bag_boxes b2  (cost=0.15..0.63 rows=1 width=36)       Index Cond:
(b1.geom&& geom)       Filter: (box_distance(b1.geom, geom) = '0'::double precision) **/ 

-- end script --

Thanks,
Regina

On 03/08/2016 01:24 AM, Regina Obe wrote:
>>> On Fri, Mar 4, 2016 at 9:29 PM, Regina Obe <lr@pcorp.us>> wrote:
>>> I think the answer to this question is NO, but thought I'd ask.
>>>
>>> A lot of folks in PostGIS land are suffering from restore issues,
>>> materialized view issues etc. because we have functions such as
>>>
>>> ST_Intersects
>>>
>>> Which does _ST_Intersects  AND &&
>>>
>>> Since _ST_Intersects is not schema qualified, during database restore
>>> (which sets the schema to the table or view schema), materialized
>>> views that depend on this do not come back.
>
>> Could you provide a self-contained, reproducible test case that illustrates this problem?  Ideally, one that doesn't
involveinstalling PostGIS?
 
>
> Here is a script just involving the built in geometric types that has the same issue:

Hi,

I think Robert was asking for a test case for the database restore problems.

The reason your no_inline() function cannot be inlined is due to lack of 
support of inlining of any functions which have any config variable set, 
not matter which. The search_path does not get any special treatment, 
and I am not sure if it could in the general case since the new search 
path will apply too to functions called by the function which changed 
the search path.

Andreas

-----Original Message-----
> From: Andreas Karlsson [mailto:andreas@proxel.se]
> Sent: Tuesday, March 08, 2016 10:43 PM
> To: Regina Obe <lr@pcorp.us>; 'Robert Haas' <robertmhaas@gmail.com>
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Is there a way around function search_path killing SQL function inlining?


> Hi,

> I think Robert was asking for a test case for the database restore problems.

> The reason your no_inline() function cannot be inlined is due to lack of support of inlining of any functions which
haveany config variable set, not matter which. The search_path does not get any special treatment, and I am not sure if
it
> could in the general case since the new search path will apply too to functions called by the function which changed
thesearch path. 

> Andreas

Restore has been an issue since as far back as I can remember.  It's more of an issue now now that people are using
materializedviews and raster constraints. 
Anytime you have a materialized view or check constraint on a table that  uses a function that calls a non-schema
qualifiedfunction you have a problem. 

For a simple example lets say you created a database like this:
-- code start here --
CREATE DATABASE test;
ALTER DATABASE test SET search_path = public,funcs;

\connect test;
CREATE SCHEMA funcs;
CREATE OR REPLACE FUNCTION funcs._helper(box, box) RETURNS float8 AS
$$ SELECT box_distance($1,$2);
$$
language 'sql' IMMUTABLE STRICT;
CREATE OR REPLACE FUNCTION funcs.inline(box,box) RETURNS boolean AS
$$SELECT $1 && $2 AND _helper($1,$2) = 0;
$$
language 'sql' IMMUTABLE;


CREATE TABLE bag_boxes(id serial primary key, geom box);
CREATE INDEX idx_bag_boxes_geom ON bag_boxes USING gist(geom);

INSERT INTO bag_boxes(geom)
SELECT ('((' || i::text || ',' || j::text || '), (' || k::text || ', ' || l::text || '))')::box
FROM generate_series(1,10) i , generate_series(11,20) j, generate_series(5,10) k, generate_series(10, 15) l ;


CREATE MATERIALIZED VIEW vw_bag_boxes AS
SELECT *
FROM bag_boxes
WHERE funcs.inline('((1,2),(3,4))'::box, geom);

-- code end here --


When you back up the database, it would create a backup with this line:

SET search_path = public, pg_catalog;
--your create materialized view here

When you restore even if your database has search_paths set, your materialized view will not come back and will error
outwith: 

ERROR:  function _helper(box, box) does not exist
LINE 2:  SELECT $1 && $2 AND _helper($1,$2) = 0;                            ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
QUERY:SELECT $1 && $2 AND _helper($1,$2) = 0;

In the case of table constraints, if you have any that rely on functions like this, your data fails validation so will
notcome back. 

Ideally it would be nice if pg_dump allowed specifying additional schemas to add to the search_path.

We have a similar issue with Foreign tables, but that's probably a harder one to fix.

Anyway it seems I underestimated the many ways setting search path on functions (even ones that don't rely on anything
elseas far as I can tell) screws up performance 
Even when it doesn't affect index usage so that has to be done with caution I guess.

Thanks,
Regina

On Thu, Mar 10, 2016 at 3:21 AM, Regina Obe <lr@pcorp.us> wrote:
> When you back up the database, it would create a backup with this line:
>
> SET search_path = public, pg_catalog;
> --your create materialized view here
>
> When you restore even if your database has search_paths set, your materialized view will not come back and will error
outwith:
 
>
> ERROR:  function _helper(box, box) does not exist
> LINE 2:  SELECT $1 && $2 AND _helper($1,$2) = 0;
>                              ^
> HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
> QUERY:
>  SELECT $1 && $2 AND _helper($1,$2) = 0;

Hmm.  The meaning of funcs.inline depends on the search_path, not just
during dump restoration but all the time.  So anything uses it under a
different search_path setting than the normal one will have this kind
of problem; not just dump/restore.

I don't have a very good idea what to do about that.

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

Robert Haas <robertmhaas@gmail.com> writes:
> Hmm.  The meaning of funcs.inline depends on the search_path, not just
> during dump restoration but all the time.  So anything uses it under a
> different search_path setting than the normal one will have this kind
> of problem; not just dump/restore.

Yeah, I see no reason to claim that this is a dump/restore-specific
problem.

> I don't have a very good idea what to do about that.

The safe way to write SQL-language functions to be search-path-proof
is to schema-qualify the names in them, or to add a "SET search_path"
clause to the function definition.

The problem with the latter approach is that it defeats inlining.
I thought for a minute that maybe we could teach the planner to do
inlining anyway by parsing the function body with the adjusted
search_path, but that doesn't really preserve the same semantics
(a SET would change the environment for called functions too).

So for now, the recommendation has to be "write functions you want
to inline with schema qualifications".  If you're worried about
preserving relocatability of an extension containing such functions,
the @extschema@ feature might help.
        regards, tom lane

On Thu, Mar 10, 2016 at 11:48:41AM -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > Hmm.  The meaning of funcs.inline depends on the search_path, not just
> > during dump restoration but all the time.  So anything uses it under a
> > different search_path setting than the normal one will have this kind
> > of problem; not just dump/restore.
> 
> Yeah, I see no reason to claim that this is a dump/restore-specific
> problem.
> 
> > I don't have a very good idea what to do about that.
> 
> The safe way to write SQL-language functions to be search-path-proof
> is to schema-qualify the names in them, or to add a "SET search_path"
> clause to the function definition.
> 
> The problem with the latter approach is that it defeats inlining.
> I thought for a minute that maybe we could teach the planner to do
> inlining anyway by parsing the function body with the adjusted
> search_path, but that doesn't really preserve the same semantics
> (a SET would change the environment for called functions too).
> 
> So for now, the recommendation has to be "write functions you want
> to inline with schema qualifications".  If you're worried about
> preserving relocatability of an extension containing such functions,
> the @extschema@ feature might help.

Is this a TODO item?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

* Bruce Momjian (bruce@momjian.us) wrote:
> On Thu, Mar 10, 2016 at 11:48:41AM -0500, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > Hmm.  The meaning of funcs.inline depends on the search_path, not just
> > > during dump restoration but all the time.  So anything uses it under a
> > > different search_path setting than the normal one will have this kind
> > > of problem; not just dump/restore.
> >
> > Yeah, I see no reason to claim that this is a dump/restore-specific
> > problem.
> >
> > > I don't have a very good idea what to do about that.
> >
> > The safe way to write SQL-language functions to be search-path-proof
> > is to schema-qualify the names in them, or to add a "SET search_path"
> > clause to the function definition.
> >
> > The problem with the latter approach is that it defeats inlining.
> > I thought for a minute that maybe we could teach the planner to do
> > inlining anyway by parsing the function body with the adjusted
> > search_path, but that doesn't really preserve the same semantics
> > (a SET would change the environment for called functions too).
> >
> > So for now, the recommendation has to be "write functions you want
> > to inline with schema qualifications".  If you're worried about
> > preserving relocatability of an extension containing such functions,
> > the @extschema@ feature might help.
>
> Is this a TODO item?

For my 2c, yes, but what we actually need is a way to reference
functions in *other* extensions, which might wish to be relocatable.

Even if they don't wish to be relocatable, we don't have any way to say
"replace this string with the schema name of this extension".  In other
words, we need something like:

@extschema{'postgis'}@

Which could then be used by extensions that depend on the PostGIS
extension to fully-qualify their function calls.

Thanks!

Stephen

On Thu, Mar 10, 2016 at 11:48:41AM -0500, Tom Lane wrote:
> If you're worried about preserving relocatability of an extension
> containing such functions, the @extschema@ feature might help.

As I've been bitten by this problem recently, I thought I'd take a look
at editing the PostGIS extension SQL file to this end, but contrary to
the above, the @extschema@ feature only applies to non-relocatable
extensions, from src/backend/commands/extension.c:
 * If it's not relocatable, substitute the target schema name for * occurrences of @extschema@. * * For a relocatable
extension,we needn't do this.  There cannot be * any need for @extschema@, else it wouldn't be relocatable.
 

I'm not sure that logic is sound - even if setting @extschema@ 
explicitly in the SQL functions bodies kills inlining (not sure about
that) or wouldn't help for other reasons, ISTM this should be 
reconsidered in the light of the use case with materialized views during
restore.


Best regards,

Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Michael Banck <michael.banck@credativ.de> writes:
> As I've been bitten by this problem recently, I thought I'd take a look
> at editing the PostGIS extension SQL file to this end, but contrary to
> the above, the @extschema@ feature only applies to non-relocatable
> extensions, from src/backend/commands/extension.c:

>   * If it's not relocatable, substitute the target schema name for
>   * occurrences of @extschema@.
>   *
>   * For a relocatable extension, we needn't do this.  There cannot be
>   * any need for @extschema@, else it wouldn't be relocatable.

> I'm not sure that logic is sound - even if setting @extschema@ 
> explicitly in the SQL functions bodies kills inlining (not sure about
> that) or wouldn't help for other reasons, ISTM this should be 
> reconsidered in the light of the use case with materialized views during
> restore.

It's not simply a matter of allowing the substitution to occur while
reading the extension script.  "Relocatable" means that we support
ALTER EXTENSION SET SCHEMA, which means moving all the extension's objects
into some new schema.  There's no good way to run around and find places
where @extschema@ was replaced in order to change them to something else.

Basically the point of @extschema@ is to support extensions that are
relocatable at installation time, but not afterwards.
        regards, tom lane

> Michael Banck <michael.banck@credativ.de> writes:
>> As I've been bitten by this problem recently, I thought I'd take a 
>> look at editing the PostGIS extension SQL file to this end, but 
>> contrary to the above, the @extschema@ feature only applies to 
>> non-relocatable extensions, from src/backend/commands/extension.c:

>>   * If it's not relocatable, substitute the target schema name for
>>  * occurrences of @extschema@.
>>   *
>>   * For a relocatable extension, we needn't do this.  There cannot be
>>   * any need for @extschema@, else it wouldn't be relocatable.

>> I'm not sure that logic is sound - even if setting @extschema@ 
>> explicitly in the SQL functions bodies kills inlining (not sure about
>> that) or wouldn't help for other reasons, ISTM this should be 
>> reconsidered in the light of the use case with materialized views 
> > during restore.

> It's not simply a matter of allowing the substitution to occur while
reading the extension script.  "Relocatable" means that we support ALTER
EXTENSION SET SCHEMA, which means moving all the 
> extension's objects into some new schema.  There's no good way to run
around and find places where @extschema@ was replaced in order to change
them to something else.

> Basically the point of @extschema@ is to support extensions that are
relocatable at installation time, but not afterwards.

>            regards, tom lane

FWIW on upcoming PostGIS 2.3, we have changed to not allow PostGIS to be
relocatable and schema qualifying internal calls. I took Tom's suggestion of
just using @extschema@
Which did mean we needed to not allow PostGIS to be relocatable anymore.  A
bit of a bummer.

Setting search_path on functions aside from killing inlining also killed
performance in other ways so that was a no go. Not sure if that is a known
issue or not and I haven't determined under what circumstances setting
search_path kills performance when index usage does not come into play.
I'll take it as a known.
Here is an example of such a case. 

https://trac.osgeo.org/postgis/ticket/3611

Now getting to the fact that using @extschema@ means requiring extension not
to be relocatable, that was a bummer and something we would need to deal
with if we ever forced everyone to install PostGIS in a specific schema so
that other extensions that rely on us can just know where PostGIS is
installed (or as Steve Frost suggested a way for dependency extensions to be
able to specify location of dependent extensions with a code such as
@extschema_postgis@ as we've got a bunch of extensions we are aware of
relying on postgis already (pgrouting, postgis_sfcgal, postgis_topology,
postgis_tiger_geocoder)

It would also be nice if the extension model had a way to allow the
extension authors the choice of handling the 'ALTER EXTENSION SET SCHEMA'
event short of monkeying with event triggers.

Yes we really need an extensions authors list to iron out and hear about
these pain points.  :)

Thanks,
Regina

On Thu, Mar 10, 2016 at 11:48 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Hmm.  The meaning of funcs.inline depends on the search_path, not just
>> during dump restoration but all the time.  So anything uses it under a
>> different search_path setting than the normal one will have this kind
>> of problem; not just dump/restore.
>
> Yeah, I see no reason to claim that this is a dump/restore-specific
> problem.
>
>> I don't have a very good idea what to do about that.
>
> The safe way to write SQL-language functions to be search-path-proof
> is to schema-qualify the names in them, or to add a "SET search_path"
> clause to the function definition.
>
> The problem with the latter approach is that it defeats inlining.
> I thought for a minute that maybe we could teach the planner to do
> inlining anyway by parsing the function body with the adjusted
> search_path, but that doesn't really preserve the same semantics
> (a SET would change the environment for called functions too).

Let's introduce a new variant of SET that only affects the lexical
scope of the function to which it is attached, and then do what you
said.  That would be full of win, because actually I think in nearly
every case that's the behavior people actually want.  There's a reason
why (for example) Perl started out with dynamic scoping (local) and
then eventually introduced lexical scoping (my): it's because lexical
scoping makes writing correct programs easier to a greater degree than
dynamic scoping.  That's basically the same problem we're hitting here
- and not only here.

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

Robert Haas <robertmhaas@gmail.com> writes:
> Let's introduce a new variant of SET that only affects the lexical
> scope of the function to which it is attached, and then do what you
> said.  That would be full of win, because actually I think in nearly
> every case that's the behavior people actually want.

Hm.  I think that sounds a lot easier than it actually is.  As an example,
this would mean that we'd want such a search_path setting to apply during
parse analysis of a function's body, but not during planning, because it
should not apply during inlining or const-folding of another function.
On the other hand, if someone tried to "SET enable_seqscan = off" with
this new scope (a highly reasonable thing to do), that certainly should
apply during planning.

It might be practical to make it work, but it will be ticklish to
get the scope of the settings to be non-surprising.
        regards, tom lane

On Fri, Aug 12, 2016 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Let's introduce a new variant of SET that only affects the lexical
>> scope of the function to which it is attached, and then do what you
>> said.  That would be full of win, because actually I think in nearly
>> every case that's the behavior people actually want.
>
> Hm.  I think that sounds a lot easier than it actually is.  As an example,
> this would mean that we'd want such a search_path setting to apply during
> parse analysis of a function's body, but not during planning, because it
> should not apply during inlining or const-folding of another function.
> On the other hand, if someone tried to "SET enable_seqscan = off" with
> this new scope (a highly reasonable thing to do), that certainly should
> apply during planning.

Mmm.  Maybe this hypothetical new facility should confine itself to
search_path specifically.

> It might be practical to make it work, but it will be ticklish to
> get the scope of the settings to be non-surprising.

Yeah, it's certainly not the sort of thing I'm going to crank out
before breakfast some morning.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Aug 12, 2016 at 3:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  I think that sounds a lot easier than it actually is.  As an example,
>> this would mean that we'd want such a search_path setting to apply during
>> parse analysis of a function's body, but not during planning, because it
>> should not apply during inlining or const-folding of another function.
>> On the other hand, if someone tried to "SET enable_seqscan = off" with
>> this new scope (a highly reasonable thing to do), that certainly should
>> apply during planning.

> Mmm.  Maybe this hypothetical new facility should confine itself to
> search_path specifically.

Well, that would be sad, because examples like enable_seqscan seem about
as useful as search_path.

Assuming that the implementation involved something like calling guc.c to
tell it to push and later pop a new semantic level (within which outer
single-level GUC settings don't apply), it's possible that the problem
mentioned above could be solved by having the planner push a semantic
level during inline_function and simplify_function.  In normal execution
you could perhaps do it in fmgr.c's handlers.  Not sure whether we'd
need to have spi.c know about it in order to make PL functions work
nicely.
        regards, tom lane