Re: DISCARD ALL ; stored procedures

Greetings,

Looking at discard all, I was a bit suprised that 'DISCARD PLANS;'
doesn't clear out cached stored procedures. To be honest, that's one
of the main reasons I'd see to use it. I thought there had been some
discussion in the archives related to invalidating stored procedure
plans due to catalog or other changes, I would have thought it'd be
possible to hook into that to do the same on a DISCARD PLANS;.

Thoughts? Is there an issue doing that? It certainly seems like it'd
be a lot better than what he current documentation requires:

When necessary, the cache can be flushed by starting a fresh database
session.

Maybe we could use 'DISCARD PLPLANS;' or something, if people feel
there needs to be a seperate way to clear those.

Thanks,

Stephen

On Thu, Jan 6, 2011 at 3:20 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Greetings,
>
>  Looking at discard all, I was a bit suprised that 'DISCARD PLANS;'
>  doesn't clear out cached stored procedures.  To be honest, that's one
>  of the main reasons I'd see to use it.  I thought there had been some
>  discussion in the archives related to invalidating stored procedure
>  plans due to catalog or other changes, I would have thought it'd be
>  possible to hook into that to do the same on a DISCARD PLANS;.
>
>  Thoughts?  Is there an issue doing that?  It certainly seems like it'd
>  be a lot better than what he current documentation requires:
>
>  When necessary, the cache can be flushed by starting a fresh database
>  session.
>
>  Maybe we could use 'DISCARD PLPLANS;' or something, if people feel
>  there needs to be a seperate way to clear those.

this is a problem. under what circumstances would you want to discard
them and why? the main problem I see with cached plpgsql plans is
interactions with search_path -- but DISCARD might not be the best way
to attack that problem. There might be other reasons though.

merlin

* Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
> this is a problem. under what circumstances would you want to discard
> them and why? the main problem I see with cached plpgsql plans is
> interactions with search_path -- but DISCARD might not be the best way
> to attack that problem. There might be other reasons though.

interaction w/ search_path (or, rather, lack of respect for it..) is
exactly the issue here for me.

Thanks,

Stephen

On Thu, Jan 6, 2011 at 4:30 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
>> this is a problem. under what circumstances would you want to discard
>> them and why?  the main problem I see with cached plpgsql plans is
>> interactions with search_path -- but DISCARD might not be the best way
>> to attack that problem.  There might be other reasons though.
>
> interaction w/ search_path (or, rather, lack of respect for it..) is
> exactly the issue here for me.

this has been discussed a couple of times -- a plausible alternative
might be to adjust the plan caching mechanism to organize the plan
cache around search_path. that way you get a separate plan per
search_path instance.

discard has zero backwards compatibility issues but has one big
problem -- if you are using combination of connection pooling, lots of
plpgsql and search_path manipulation, you take a big performance hit.
in other words, even if you can discard everything., do you really
want to?

merlin

* Merlin Moncure (mmoncure(at)gmail(dot)com) wrote:
> this has been discussed a couple of times -- a plausible alternative
> might be to adjust the plan caching mechanism to organize the plan
> cache around search_path. that way you get a separate plan per
> search_path instance.

That would certainly be fine for me. To be honest, I feel like I've
even suggested that in the past, somewhere.

> discard has zero backwards compatibility issues but has one big
> problem -- if you are using combination of connection pooling, lots of
> plpgsql and search_path manipulation, you take a big performance hit.
> in other words, even if you can discard everything., do you really
> want to?

I don't see how this can be an unnecessary performance hit. You might
argue that I should redesign things to not work this way, but that's a
whole different discussion. At the moment the options are:

- switch to using execute
- force a full database reconnect

To get the 'correct' behavior.

If it's "performance" vs. "correctness", you can guess what I'm going to
vote for, however, in this case, I can't see how either of the other
options would perform better than a discard-like approach. If people
are already using 'discard all;' then they're already throwing away
their plans for prepared queries, it strikes me as unlikely that they'd
have an issue with also getting rid of stored procedure plans. If they
do, they could certainly use the individual 'discard' statements
instead (presuming we implement this with a new discard argument).

Thanks

Stephen

On Thu, Jan 6, 2011 at 5:22 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> If it's "performance" vs. "correctness", you can guess what I'm going to
> vote for, however, in this case, I can't see how either of the other
> options would perform better than a discard-like approach.  If people
> are already using 'discard all;' then they're already throwing away
> their plans for prepared queries, it strikes me as unlikely that they'd
> have an issue with also getting rid of stored procedure plans.  If they
> do, they could certainly use the individual 'discard' statements
> instead (presuming we implement this with a new discard argument).

If DISCARD ALL doesn't flush this stuff, I'd consider that an outright
bug. Does it?

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> If DISCARD ALL doesn't flush this stuff, I'd consider that an outright
> bug. Does it?

No, it does not, based on my testing against 8.4.5:

Simple function:

----------------------------
CREATE OR REPLACE FUNCTION test_func() RETURNS boolean
AS $_$
DECLARE
rec RECORD;
BEGIN

SELECT INTO rec statefp FROM edges LIMIT 1;

raise notice 'rec: %', rec;

RETURN TRUE;
END;
$_$ LANGUAGE plpgsql;
----------------------------

Couple of tables, note the CHECK constraints on statefp,
which is what the function pulls out:
----------------------------
gis*=> \d tiger_02.addrfn
Table "tiger_01.addrfn"
Column | Type | Modifiers
----------+-----------------------+------------------------------------------------------
gid | integer | not null default nextval('addrfn_gid_seq'::regclass)
arid | character varying(22) |
linearid | character varying(22) |
statefp | character varying(2) | not null default '01'::character varying
Indexes:
"addrfn_pkey" PRIMARY KEY, btree (gid)
Check constraints:
"addrfn_statefp_check" CHECK (statefp::text = '01'::text)
Inherits: tiger_us.addrfn
----------------------------

----------------------------
gis*=> \d tiger_02.addrfn
Table "tiger_02.addrfn"
Column | Type | Modifiers
----------+-----------------------+------------------------------------------------------
gid | integer | not null default nextval('addrfn_gid_seq'::regclass)
arid | character varying(22) |
linearid | character varying(22) |
statefp | character varying(2) | not null default '02'::character varying
Indexes:
"addrfn_pkey" PRIMARY KEY, btree (gid)
Check constraints:
"addrfn_statefp_check" CHECK (statefp::text = '02'::text)
Inherits: tiger_us.addrfn
----------------------------

See the results:
gis=> \i ./qs.sql
CREATE FUNCTION
gis*=> set search_path to tiger_01;
SET
gis*=> set search_path to tiger_01,sfrost;
SET
gis*=> select test_func();
NOTICE: rec: (01)
test_func
-----------
t
(1 row)

gis*=> commit;
COMMIT
gis=> discard all;
DISCARD ALL
gis=> set search_path to tiger_02,sfrost;
SET
gis*=> select test_func();
NOTICE: rec: (01)
test_func
-----------
t
(1 row)

The addrfn table in the tiger_02 schema certainly can not have a
statefp of 01 due to the CHECK constraint (and it believe me, it's
right).

To be honest, I agree it's a bug, and I would *love* to have it
back-patched, but I could see an argument for it to be something
explicit from DISCARD PLANS; and would hence require a grammar
change which isn't something we'd typically back-patch.

Making it part of DISCARD PLANS; and back-patching it to 8.3 where
DISCARD was introduced would be awesome for me. :)

Thanks,

Stephen

On Fri, Jan 7, 2011 at 8:43 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> To be honest, I agree it's a bug, and I would *love* to have it
> back-patched, but I could see an argument for it to be something
> explicit from DISCARD PLANS; and would hence require a grammar
> change which isn't something we'd typically back-patch.

That argument doesn't carry much water with me, because it's hard for
me to imagine a situation where you need to discard one of these
things but discarding the other one also causes a problem. I'm also
pretty unexcited about adding more stuff to the grammar to cover such
a marginal borderline case. Adding unreserved keywords is pretty
cheap, but it's not totally free. If we were going to do it I'd
suggest DISCARD LANGUAGE PLANS or something like that, but I don't
really see a reason to go there at all.

Really it seems to me that changing the search path ought to discard
anything that might have been done differently had the search path
been different, but I don't think that's back-patch material.

> Making it part of DISCARD PLANS; and back-patching it to 8.3 where
> DISCARD was introduced would be awesome for me. :)

I'd need to look at this more closely before committing anything, but
at first blush I think that's reasonable. Have a patch?

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> Really it seems to me that changing the search path ought to discard
> anything that might have been done differently had the search path
> been different, but I don't think that's back-patch material.

I like that idea, but I agree it wouldn't be back-patchable, and I could
see arguments against it also (convoluting the GUC mechanics, etc).

> > Making it part of DISCARD PLANS; and back-patching it to 8.3 where
> > DISCARD was introduced would be awesome for me. :)
>
> I'd need to look at this more closely before committing anything, but
> at first blush I think that's reasonable. Have a patch?

Sadly, no.. To be honest, I was fully expecting a response of "that's
hard to do." I'm not sure we have any mechanics in place for throwing
away stored procedure plans, but I'll go look and see if I can come up
with something. Would *love* to get this fixed.

Thanks!

Stephen

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > > Making it part of DISCARD PLANS; and back-patching it to 8.3 where
> > > DISCARD was introduced would be awesome for me. :)
> >
> > I'd need to look at this more closely before committing anything, but
> > at first blush I think that's reasonable. Have a patch?
>
> To be honest, I was fully expecting a response of "that's hard to do."

Soo, yeah, I found the "this is hard" part. Basically, the plan
cacheing, etc, is all handled by the stored procedures themselves, and
we havn't got any way to tell a PL "destroy all your plans." We might
be able to hack up fmgr to throw away all references to functions, but
that wouldn't release the memory they use up, making 'discard plans;'
leak like a sieve.

What's worse is that most PLs actually *also* allocate a bunch of stuff
in TopMemoryContext, meaning even if we figured out what contexts are
used by the PLs, we still couldn't nuke them. Personally, I'm not a fan
of the PLs doing that (perhaps there's some performance reason? dunno).

A few approaches come to mind for dealing with this-

#1. Add a new 'Top-but-removed-on-DISCARD' context and modify the PLs to
use that instead of TopMemoryContext and require any other contexts
they create to be children of it.

#2. Add another entry point to the PLs in pg_pltemplate.h which is a
function to be called on DISCARD.

#3. Add a way for PLs to request a context similar to #1, but would be
on a per-PL basis. This would involve adding a new function to
mmgr/, or adding a new parameter for creating a context.

I like the idea of having a context-per-PL, but I don't know that I can
justify the complications necessary to make it happen. Given that
they're all already dumping things in TopMemoryContext, at least moving
them out of *that* would improve the situation, so my inclination is to
do #1. Of course, user-added PLs wouldn't get this benefit and would
still leak memory after a DISCARD until they're updated.

After the above is figured out, we should be able to go through and
make fmgr get cleaned up after a DISCARD.

Thoughts?

Stephen

On Fri, Jan 7, 2011 at 1:29 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> #1. Add a new 'Top-but-removed-on-DISCARD' context and modify the PLs to
>    use that instead of TopMemoryContext and require any other contexts
>        they create to be children of it.

I'm guessing that just resetting the memory context is going to result
in things breaking all over the place - the PL might have dangling
pointers into the context. And it might have other resources that we
don't know about. Thus I think we need:

> #2. Add another entry point to the PLs in pg_pltemplate.h which is a
>    function to be called on DISCARD.

...except I think that the principal thing you need to modify is
pl_language, rather than pl_pltemplate.

If we go this route, then (1) it can't be back-patched, obviously, and
(2) we need to think a little bit harder about what we're asking to
have discarded, because I think it's going to be a lot more than just
cached plans.

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

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Fri, Jan 7, 2011 at 1:29 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > #1. Add a new 'Top-but-removed-on-DISCARD' context and modify the PLs to
> >    use that instead of TopMemoryContext and require any other contexts
> >        they create to be children of it.
>
> I'm guessing that just resetting the memory context is going to result
> in things breaking all over the place - the PL might have dangling
> pointers into the context.

After looking through the code more, we actually already say "use this
context for stuff you allocate in fn_extra", but it doesn't look like
the PLs are respecting or using that. We do have a function which
resets fn_extra already (fmgr_finfo_copy) but I'm not sure under what
conditions it's used and I'm not sure why it doesn't leak memory by
doing that.

If we can figure out the list of functions that have been called, get at
all of their fn_extra pointers to set them to NULL, and nuke the context
that they're created in, that should work. The PLs in core appear to be
good about using fn_extra and resetting it should be sufficient to force
a recompile of the stored procedures. It also looks like they shouldn't
have any issue surviving that reset.

> And it might have other resources that we
> don't know about. Thus I think we need:

This is certainly a concern and would be a reason to offer a seperate
function for the PLs to use, but I'm not sure we need to jump there
right away. I'd like to see if the core/contrib PLs can all handle the
above approach and then see if third-party PLs complain.

> > #2. Add another entry point to the PLs in pg_pltemplate.h which is a
> >    function to be called on DISCARD.
>
> ...except I think that the principal thing you need to modify is
> pl_language, rather than pl_pltemplate.

Right, sorry.

> If we go this route, then (1) it can't be back-patched, obviously, and
> (2) we need to think a little bit harder about what we're asking to
> have discarded, because I think it's going to be a lot more than just
> cached plans.

I'm not ready to give up quite yet, but I agree that we might end up
there.

Thanks,

Stephen

All,

Alright, so, the whole fn_extra stuff seems to be unrelated.. I'm not
sure when it's used (perhaps multiple calls to the same function in a
given query?), but the PLs have their own hash tables that they use for
storing functions that have been called. I had assumed that was done
through fmgr, but apparently not (or at least, I can't find where..).
I'm starting to wonder if we're trying to do too much with this
though. If all the PLs have to go through SPI to *get* plans (at least
ones we care about), perhaps we could just use SPI to implement the
plan invalidation?

Consider if we saved the DISCARD's transaction ID and store the
last-discard txn (or whenever the function was first prepared) in the
result of the SPI prepare and then detect if we need to switch to
replanning the query in SPI_execute_plan instead of just executing it.
Of course, we'd have to have enough info *to* replan it, but we should
be able to manage that.

Thoughts?

Stephen

On Sat, Jan 8, 2011 at 4:28 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Thoughts?

Unfortunately, we've officially exceeded my level of knowledge to the
point where I can't comment intelligently. Sorry.... :-(

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

Excerpts from Stephen Frost's message of vie ene 07 15:29:52 -0300 2011:
> * Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > > > Making it part of DISCARD PLANS; and back-patching it to 8.3 where
> > > > DISCARD was introduced would be awesome for me. :)
> > >
> > > I'd need to look at this more closely before committing anything, but
> > > at first blush I think that's reasonable. Have a patch?
> >
> > To be honest, I was fully expecting a response of "that's hard to do."
>
> Soo, yeah, I found the "this is hard" part. Basically, the plan
> cacheing, etc, is all handled by the stored procedures themselves, and
> we havn't got any way to tell a PL "destroy all your plans." We might
> be able to hack up fmgr to throw away all references to functions, but
> that wouldn't release the memory they use up, making 'discard plans;'
> leak like a sieve.

What this discussion suggests to me is that cached plans need to be
tracked by a resource owner that's linked to the function. The problem
I see with this idea is figure out what module would keep track of
resowners that need to be reset ... Other than that I think it should
be straightforward :-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support