Обсуждение: DISCARD ALL (Again)

Hello,

Over at my quaint establishment we have been working on some plython 
work that makes use of GD. We wrote this code with the assumption (per 
the docs) that when you issued a DISCARD ALL, the GD would be cleared. 
Apparently this is not the case. The docs themselves are clearly wrong, 
and this has been brought up before multiple times (that DISCARD ALL 
doesn't do what people think it does).

Does it seem reasonable based on the docs:

DISCARD ALL:

Releases all temporary resources associated with the current session and 
resets the session to its initial state.

That we should also release the GD?

Sincerely,

Joshua D. Drake


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.

On 04/17/2014 01:44 PM, Joshua D. Drake wrote:
> Does it seem reasonable based on the docs:
> 
> DISCARD ALL:
> 
> Releases all temporary resources associated with the current session and
> resets the session to its initial state.
> 
> That we should also release the GD?

It does, but that's a feature request, not a bug.  Same with pl/perl.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 04/17/2014 02:17 PM, Josh Berkus wrote:
>
> On 04/17/2014 01:44 PM, Joshua D. Drake wrote:
>> Does it seem reasonable based on the docs:
>>
>> DISCARD ALL:
>>
>> Releases all temporary resources associated with the current session and
>> resets the session to its initial state.
>>
>> That we should also release the GD?
>
> It does, but that's a feature request, not a bug.  Same with pl/perl.
>

Well I would argue it is a documentation bug/error and that yes, we have 
a legitimate feature request for DISCARD ALL to clear the GD.

Sincerely,

JD

-- 
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.

Joshua D. Drake wrote:
> 
> On 04/17/2014 02:17 PM, Josh Berkus wrote:
> >
> >On 04/17/2014 01:44 PM, Joshua D. Drake wrote:
> >>Does it seem reasonable based on the docs:
> >>
> >>DISCARD ALL:
> >>
> >>Releases all temporary resources associated with the current session and
> >>resets the session to its initial state.
> >>
> >>That we should also release the GD?
> >
> >It does, but that's a feature request, not a bug.  Same with pl/perl.
> >
> 
> Well I would argue it is a documentation bug/error and that yes, we
> have a legitimate feature request for DISCARD ALL to clear the GD.

It does sounds a legitimate feature request to me.  I don't remember if
we honored the request to add resetting of cached sequences, though; if
we didn't, this one is probably going to be tough too.

Another point is that to implement this I think there will need to be
another per-PL entry point to discard session data; are we okay with
that?  Since this probably means a new column in pg_language, we
couldn't even consider the idea of back-patching.  Unless we add a hook,
which is registered in the PL's _PG_init()?

Are we going to backpatch a doc change that says "releases all temporary
resources, except for plptyhon's and plperl's GD"?  Surely not ...

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera-9 wrote
> Are we going to backpatch a doc change that says "releases all temporary
> resources, except for plptyhon's and plperl's GD"?  Surely not ...

GD = Global Dictionary

I don't see why something like the following wouldn't have value.

"For those languages that make use of a Global Dictionary (or other global)
construct to maintain state across function calls the content of these
global constructs may not be reset upon a call to DISCARD ALL - please see
your language's documentation for specifics.".

Then, more attention should be made within the language sections both to the
constructs in general and also their limitations in the face of DISCARD ALL.

"Note, the session-level GD construct is not informed of the execution of
DISCARD ALL and thus is not cleared when such is executed."

It would be helpful to note any work-around options as well.

It sounds like, generally, PostgreSQL should promise to notify each
language/session of a pending DISCARD ALL and then leave it up to the
language to define how it will respond.  The documentation should reflect
this promise/action division-of-responsibilities.

David J.












--
View this message in context: http://postgresql.1045698.n5.nabble.com/DISCARD-ALL-Again-tp5800623p5800640.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

"Joshua D. Drake" <jd@commandprompt.com> writes:
> Does it seem reasonable based on the docs:
> DISCARD ALL:
> Releases all temporary resources associated with the current session and 
> resets the session to its initial state.
> That we should also release the GD?

There are a couple of reasons why this isn't anywhere near as
open-and-shut as you seem to think:

1. The things that DISCARD ALL resets are explicitly enumerated in its
documentation page; it is not an open-ended promise to clean up anything
anybody happens to think of.  In particular, I would object to adding any
such functionality that doesn't correspond to a more specific form of
DISCARD, RESET, or whatever.

2. While I'm no Python expert, I believe GD is just a specific instance
of a general capability for global state in Python.  Are we going to
promise that any and all user-created data inside Python goes away?
What about other PLs?  Will users thank us if this suddenly starts
happening?

3. Since PL/Python is a superuser-only language, destroying either all
global state or just GD should require superuser privileges.  It will
definitely break things to make DISCARD ALL require superuser privs.
It's not very much more attractive to say that it only destroys Python
state if issued by a superuser.

We could in fact implement #2, I imagine, by destroying and recreating
the entire language interpreter.  So I could imagine implementing a
"DISCARD INTERPRETERS" kind of command that would zap the current
interpreter(s) for whichever PL languages happened to feel like
cooperating with the command.  But I'm not at all sure about the
superuser-privileges aspect of that.

More: IIRC, in at least some of the PLs, the interpreters are
SQL-user-specific.  Should a non-superuser session user have the ability
to destroy state in interpreters running for other user IDs (presumably,
state created by SECURITY DEFINER functions)?  I'd guess not.

On the whole I'm not sure this is something we ought to get into.
If you really need a fresh session, maybe you should start a
fresh session.  But if we are going to get into it, I think the
permissions aspect needs some careful thought.
        regards, tom lane

On 04/17/2014 05:24 PM, Tom Lane wrote:
> "Joshua D. Drake" <jd@commandprompt.com> writes:
>> Does it seem reasonable based on the docs:
>> DISCARD ALL:
>> Releases all temporary resources associated with the current session and
>> resets the session to its initial state.
>> That we should also release the GD?
>
> There are a couple of reasons why this isn't anywhere near as
> open-and-shut as you seem to think:
>
> 1. The things that DISCARD ALL resets are explicitly enumerated in its
> documentation page; it is not an open-ended promise to clean up anything
> anybody happens to think of.

Actually, it is. Unless we change the wording in the documentation 
because right now, DISCARD ALL states that it will return the session 
back to its initial state. It does not do that.

[... snipping technical details for those who would be better at arguing 
than I ... ]

>
> On the whole I'm not sure this is something we ought to get into.
> If you really need a fresh session, maybe you should start a
> fresh session.

This isn't nearly as simple as it sounds. That said, the work around we 
came up with was to have server_reset_query in pg_bouncer call a 
function called discard_gd() that explicitly discards the GD for that 
session.

> But if we are going to get into it, I think the
> permissions aspect needs some careful thought.
>

Agreed.

Sincerely,

JD

-- 
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.

"Joshua D. Drake" <jd@commandprompt.com> writes:
> On 04/17/2014 05:24 PM, Tom Lane wrote:
>> 1. The things that DISCARD ALL resets are explicitly enumerated in its
>> documentation page; it is not an open-ended promise to clean up anything
>> anybody happens to think of.

> Actually, it is. Unless we change the wording in the documentation 

Well, we definitely need to tweak the short description of DISCARD ALL.
But the long description a bit further down the page is quite clear.
        regards, tom lane

On 04/17/2014 05:24 PM, Tom Lane wrote:

> On the whole I'm not sure this is something we ought to get into.
> If you really need a fresh session, maybe you should start a
> fresh session.

* David G Johnston (david.g.johnston@gmail.com) wrote:
> > On 04/17/2014 05:24 PM, Tom Lane wrote:
> > > On the whole I'm not sure this is something we ought to get into.
> > > If you really need a fresh session, maybe you should start a
> > > fresh session.
> >
> Isn't the whole point to avoid the reconnection overhead, especially for
> connection poolers?  DISCARD ALL shouldn't cause any cleanup that wouldn't
> otherwise occur when a session disconnects.  True global data (not just
> session global) should be excluded.

Err, this is per-backend global data, so it will get cleaned up on
session disconnect.

> A better wording of the promise would be: "discard all" leaves the session
> in the same state it would be in if the underlying connection were dropped
> and re-established.

This is not accurate.
Thanks,
    Stephen

On 04/17/2014 07:07 PM, David G Johnston wrote:
>
>     On 04/17/2014 05:24 PM, Tom Lane wrote:
>      > On the whole I'm not sure this is something we ought to get into.
>      > If you really need a fresh session, maybe you should start a
>      > fresh session.
>
>
> Isn't the whole point to avoid the reconnection overhead, especially for
> connection poolers?  DISCARD ALL shouldn't cause any cleanup that
> wouldn't otherwise occur when a session disconnects.  True global data
> (not just session global) should be excluded.

The GD is global to the session only (Like temp tables).

>
> A better wording of the promise would be: "discard all" leaves the
> session in the same state it would be in if the underlying connection
> were dropped and re-established.

Except that it doesn't.

JD


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.

On 04/17/2014 07:07 PM, David G Johnston wrote:

    On 04/17/2014 05:24 PM, Tom Lane wrote:
     > On the whole I'm not sure this is something we ought to get into.
     > If you really need a fresh session, maybe you should start a
     > fresh session.


Isn't the whole point to avoid the reconnection overhead, especially for
connection poolers?  DISCARD ALL shouldn't cause any cleanup that
wouldn't otherwise occur when a session disconnects.  True global data
(not just session global) should be excluded.

The GD is global to the session only (Like temp tables).

A better wording of the promise would be: "discard all" leaves the
session in the same state it would be in if the underlying connection
were dropped and re-established.

Except that it doesn't.

On 4/17/14, 8:24 PM, Tom Lane wrote:
> We could in fact implement #2, I imagine, by destroying and recreating
> the entire language interpreter.  So I could imagine implementing a
> "DISCARD INTERPRETERS" kind of command that would zap the current
> interpreter(s) for whichever PL languages happened to feel like
> cooperating with the command.

More generally, any extension could maintain any kind of cross-call
state.  plproxy, dblink, pgmemcache come to mind.  A general hook into
DISCARD might be doable, but then it's not clear how to categorize this
into DISCARD subcommands.

On 4/17/14, 4:44 PM, Joshua D. Drake wrote:
> That we should also release the GD?

In some cases, SD or GD are used to cache things.  Having the connection
pooler blow that away would defeat the point.

Peter Eisentraut <peter_e@gmx.net> writes:
> On 4/17/14, 8:24 PM, Tom Lane wrote:
>> We could in fact implement #2, I imagine, by destroying and recreating
>> the entire language interpreter.  So I could imagine implementing a
>> "DISCARD INTERPRETERS" kind of command that would zap the current
>> interpreter(s) for whichever PL languages happened to feel like
>> cooperating with the command.

> More generally, any extension could maintain any kind of cross-call
> state.  plproxy, dblink, pgmemcache come to mind.  A general hook into
> DISCARD might be doable, but then it's not clear how to categorize this
> into DISCARD subcommands.

Right.  So if we go down that path, we're basically giving up the ability
to define what DISCARD ALL does by specifying an equivalent list of
subcommands.  Maybe that's an acceptable tradeoff, but I don't like it
much.

I wonder whether we could have a "DISCARD identifier" variant where
any hooked-in extension could recognize the identifier and zap some
appropriate subset of its own state.  This would get us to a situation
where we could say "DISCARD ALL is the union of all DISCARD operations,
but some of those are documented in relevant extensions' docs rather
than here".

The permissions problem remains, of course.
        regards, tom lane

On 04/18/2014 08:01 AM, Peter Eisentraut wrote:
>
> On 4/17/14, 4:44 PM, Joshua D. Drake wrote:
>> That we should also release the GD?
>
> In some cases, SD or GD are used to cache things.  Having the connection
> pooler blow that away would defeat the point.

Not on a per session basis. Although I can see your point. The GD is 
supposed to be global per session. If, I discard the session to it's 
original state, that is going to predate the creation of the GD. That is 
expected behavior.

JD


>
>
>


-- 
Command Prompt, Inc. - http://www.commandprompt.com/  509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
Political Correctness is for cowards.

<div dir="ltr"><div class="gmail_extra"><br />On Fri, Apr 18, 2014 at 12:00 PM, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> On 4/17/14, 8:24 PM, Tom Lane wrote:<br
/>>> We could in fact implement #2, I imagine, by destroying and recreating<br /> > > the entire language
interpreter. So I could imagine implementing a<br />> > "DISCARD INTERPRETERS" kind of command that would zap the
current<br/>> > interpreter(s) for whichever PL languages happened to feel like<br /> > > cooperating with
thecommand.<br />><br />> More generally, any extension could maintain any kind of cross-call<br />> state.
 plproxy,dblink, pgmemcache come to mind.  A general hook into<br />> DISCARD might be doable, but then it's not
clearhow to categorize this<br /> > into DISCARD subcommands.<br />><br /><br /></div><div class="gmail_extra">I
proposedsome like that in a previous message [1].<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Regards,<br/></div><div class="gmail_extra"><br />[1] <a
href="http://www.postgresql.org/message-id/CAFcNs+p+PMFhkiPTF1Xbz3neG8d0MtEnb021H2FUG049333LrQ@mail.gmail.com">http://www.postgresql.org/message-id/CAFcNs+p+PMFhkiPTF1Xbz3neG8d0MtEnb021H2FUG049333LrQ@mail.gmail.com</a><br
/><br/>--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

On Fri, Apr 18, 2014 at 2:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 2. While I'm no Python expert, I believe GD is just a specific instance
> of a general capability for global state in Python.  Are we going to
> promise that any and all user-created data inside Python goes away?
> What about other PLs?  Will users thank us if this suddenly starts
> happening?

This is not the first time that somebody's asked for a way to throw
away global interpreter state, and I really think we ought to oblige.
In a connection-pooling environment, you really need a way to get the
connection back to its original state rather than some not-so-near
facsimile thereof.  Maybe it'll end up as an optional behavior, and
which kind of reset to use will become part of the pooler
configuration, but that doesn't bother me as much as not having it for
those that want it.

What's a bit odd about this request is that it asks for the ability to
throw away only part of the state.  ISTM that if somebody wants to add
that kind of capability, they ought to just package a function which
does precisely that with the plpython extension, or create a Python
function that zaps that particular variable if that's possible.  I
think it's clearly useful to have DISCARD ALL be a request to discard
*everything* in one shot, but it's going to be a stretch to come up
with DISCARD variants for every kind of partial state removal somebody
wants to do.

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

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Apr 18, 2014 at 2:24 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 2. While I'm no Python expert, I believe GD is just a specific instance
> > of a general capability for global state in Python.  Are we going to
> > promise that any and all user-created data inside Python goes away?
> > What about other PLs?  Will users thank us if this suddenly starts
> > happening?
>
> This is not the first time that somebody's asked for a way to throw
> away global interpreter state, and I really think we ought to oblige.
> In a connection-pooling environment, you really need a way to get the
> connection back to its original state rather than some not-so-near
> facsimile thereof.  Maybe it'll end up as an optional behavior, and
> which kind of reset to use will become part of the pooler
> configuration, but that doesn't bother me as much as not having it for
> those that want it.

Drop the connection and reconnect would be the answer to that.  For as
much as we may hope and wish for a connection to go back to 'the way it
was upon first connection', throwing away the interpretor *might* (and I
wouldn't be comfortable claiming it absolutely..) get you there when
you've only called functions which use interpretors, but people write
code in C too and we've seen complaints of memory leaks, etc, from C
libraries and C extensions- and there's nothing we're going to be able
to do to address that, so this mythical 'DISCARD EVERYTHING' is a pipe
dream.  (Were we to actually re-exec ourselves into a new process, as if
we went through a disconnect/reconnect, I'd be more inclined to support
this capability, but I'm not sure what such would really buy us...)

> What's a bit odd about this request is that it asks for the ability to
> throw away only part of the state.  ISTM that if somebody wants to add
> that kind of capability, they ought to just package a function which
> does precisely that with the plpython extension, or create a Python
> function that zaps that particular variable if that's possible.  I
> think it's clearly useful to have DISCARD ALL be a request to discard
> *everything* in one shot, but it's going to be a stretch to come up
> with DISCARD variants for every kind of partial state removal somebody
> wants to do.

Agreed.
Thanks,
    Stephen

On Thu, Apr 17, 2014 at 6:51 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> It does sounds a legitimate feature request to me.  I don't remember if
> we honored the request to add resetting of cached sequences, though; if
> we didn't, this one is probably going to be tough too.
>

+1


> Another point is that to implement this I think there will need to be
> another per-PL entry point to discard session data; are we okay with
> that?  Since this probably means a new column in pg_language, we
> couldn't even consider the idea of back-patching.  Unless we add a hook,
> which is registered in the PL's _PG_init()?
>

This week I had some similar trouble, but using "dblink" and "pgbouncer".
As expected "DISCARD ALL" don't clear the extension resources.

I was thinking if is possible to every extension register his own "discard" procedure and then the "DISCARD ALL" can execute all registered extension cleanup procedures. Makes sense?

> Are we going to backpatch a doc change that says "releases all temporary
> resources, except for plptyhon's and plperl's GD"?  Surely not ...
>

Maybe this doc can be like that:

"releases all temporary resources, except for extensions"

Grettings,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On 04/18/2014 06:44 PM, Joshua D. Drake wrote:
>
> On 04/18/2014 08:01 AM, Peter Eisentraut wrote:
>>
>> On 4/17/14, 4:44 PM, Joshua D. Drake wrote:
>>> That we should also release the GD?
>>
>> In some cases, SD or GD are used to cache things.  Having the connection
>> pooler blow that away would defeat the point.
>
> Not on a per session basis. Although I can see your point.
> The GD is supposed to be global per session.
> If, I discard the session to it's original state, that is going to
> predate the creation of the GD. That is expected behavior.
The reason (I assume) you want DISCARD ALL instead of reconnect is
performance.

Often stuff (like another connection) is cached in GD also for performance.

Another things I sometimes do in pl/pythonu is add my own functions to
__builtins__
and also use pl/python functions as modules by having some "global"
state in definitions.

The only way to automatically reset all that would be complete reset of
interpreter (as Tom mentioned above)

For these reasons I suggest that the cleanest way to achieve "discard
all" for pl/python
would be ability to register a clean-up function and not trying to
second-guess what
global cached state *might* exists.


Cheers

-- 
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ