Thread: plpython is broken for recursive use
I looked into bug #13683, http://www.postgresql.org/message-id/20151015135804.3019.31908@wrigleys.postgresql.org It looks to me like plpython basically doesn't work at all for re-entrant calls to plpython functions, because all executions of a given function share the same "globals" dict, which doesn't seem sane. Shouldn't each execution have its own? I believe the specific sequence of events shown here is 1. On the way into the recursion, each level does PLy_function_build_args which sets "i" as a key in the proc's globals dict. 2. On the way out, each level does PLy_function_delete_args which deletes "i" from the dict. At levels after the innermost, this results in setting "KeyError: 'i'" as the active Python error, because that key is already gone. We fail to notice the error indicator, though. 3. The pending error indicator causes the attempt to "def" the second function to fail. If I change the test case to create or replace function a(i integer) returns integer as $$ if i > 1: return i j = plpy.execute("select a(%s)" % (i+1))[0]['a'] return i+j $$ language plpythonu; select a(0); then I get ERROR: spiexceptions.ExternalRoutineException: NameError: global name 'i' is not defined CONTEXT: Traceback (most recent call last): PL/Python function "a", line 4, in <module> j = plpy.execute("select a(%s)"% (i+1))[0]['a'] PL/Python function "a" which shows that a()'s own access to "i" is broken too, though I confess I'm not sure why it's failing at line 4 rather than 5. I think this means that we should get rid of proc->globals and instead manufacture a new globals dict locally in each call to PLy_exec_function or PLy_exec_trigger. For SETOF functions it would be necessary to keep the globals dict reference somewhere in the FunctionCallInfo struct, probably. Not sure about cleaning up after an error that occurs between SETOF callbacks --- we might need plpython to grow an at-abort callback to do decref's on unreleased dicts. Thoughts? regards, tom lane
On 10/15/2015 01:10 PM, Tom Lane wrote: > I think this means that we should get rid of proc->globals and instead > manufacture a new globals dict locally in each call to PLy_exec_function > or PLy_exec_trigger. For SETOF functions it would be necessary to keep > the globals dict reference somewhere in the FunctionCallInfo struct, > probably. Not sure about cleaning up after an error that occurs between > SETOF callbacks --- we might need plpython to grow an at-abort callback to > do decref's on unreleased dicts. Don't people currently specifically treat the state of the globals dict as a feature? That is, make use of the fact that you can store session-persistent data in it? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On 10/15/2015 05:16 PM, Josh Berkus wrote: > On 10/15/2015 01:10 PM, Tom Lane wrote: >> I think this means that we should get rid of proc->globals and instead >> manufacture a new globals dict locally in each call to PLy_exec_function >> or PLy_exec_trigger. For SETOF functions it would be necessary to keep >> the globals dict reference somewhere in the FunctionCallInfo struct, >> probably. Not sure about cleaning up after an error that occurs between >> SETOF callbacks --- we might need plpython to grow an at-abort callback to >> do decref's on unreleased dicts. > Don't people currently specifically treat the state of the globals dict > as a feature? That is, make use of the fact that you can store > session-persistent data in it? > That was the thinking behind plperl's %_SHARED, and I assume this is similar. cheers andrew
On 10/15/2015 02:16 PM, Josh Berkus wrote: > On 10/15/2015 01:10 PM, Tom Lane wrote: >> I think this means that we should get rid of proc->globals and instead >> manufacture a new globals dict locally in each call to PLy_exec_function >> or PLy_exec_trigger. For SETOF functions it would be necessary to keep >> the globals dict reference somewhere in the FunctionCallInfo struct, >> probably. Not sure about cleaning up after an error that occurs between >> SETOF callbacks --- we might need plpython to grow an at-abort callback to >> do decref's on unreleased dicts. > > Don't people currently specifically treat the state of the globals dict > as a feature? That is, make use of the fact that you can store > session-persistent data in it? Yes, just like the plperl feature. jD > -- Command Prompt, Inc. - http://www.commandprompt.com/ 503-667-4564 PostgreSQL Centered full stack support, consulting and development. New rule for social situations: "If you think to yourself not even JD would say this..." Stop and shut your mouth. It's going to be bad.
Andrew Dunstan <andrew@dunslane.net> writes: > On 10/15/2015 05:16 PM, Josh Berkus wrote: >> On 10/15/2015 01:10 PM, Tom Lane wrote: >>> I think this means that we should get rid of proc->globals and instead >>> manufacture a new globals dict locally in each call to PLy_exec_function >>> or PLy_exec_trigger. >> Don't people currently specifically treat the state of the globals dict >> as a feature? That is, make use of the fact that you can store >> session-persistent data in it? > That was the thinking behind plperl's %_SHARED, and I assume this is > similar. I poked around in the code a bit more, and I now see that the procedure's "globals" dictionary actually is global, in the sense that it's not the most closely nested namespace when the procedure's code runs. It's not so surprising that if you write "global foo" then foo will have a value that persists across calls. But then it is fair to ask what the heck is the point of the "SD" dict, which has got *exactly* the same lifespan as the procedure's globals dictionary --- if you want to share a value across multiple executions of the procedure, it does not matter whether you make it within SD or just declare it "global". So why'd we bother with SD? Anyway, the real problem here is the decision to pass procedure arguments by assigning them to keys in the global dict. That is just brain-dead, both because it means that recursive calls can't possibly work and because it creates the bizarre scoping behavior mentioned in http://www.postgresql.org/docs/devel/static/plpython-funcs.html I suppose we cannot change that in back branches for fear of creating subtle compatibility problems, but surely we can do better going forward? Another interesting point is that if the procedure cache entry is rebuilt for any reason whatever, such as an ALTER on the function definition or even just an sinval flush, you lose whatever may have been in either SD or the procedure's "global" namespace. That seems like a rather surprising implementation behavior. I'd have expected plpython to make some effort to make "SD" have actual session lifespan, not just "maybe it'll survive and maybe it won't". regards, tom lane
I wrote: > Anyway, the real problem here is the decision to pass procedure arguments > by assigning them to keys in the global dict. That is just brain-dead, > both because it means that recursive calls can't possibly work and because > it creates the bizarre scoping behavior mentioned in > http://www.postgresql.org/docs/devel/static/plpython-funcs.html > I suppose we cannot change that in back branches for fear of creating > subtle compatibility problems, but surely we can do better going forward? I looked into this a little more. The Python function we use to execute plpython code, PyEval_EvalCode, has arguments to supply both a global and a local namespace dict. Right now we just pass proc->globals for both, which sounds and is weird. However, what we're actually executing in that context is just an argument-less function call, eg "__plpython_procedure_foo_nnn()". So I believe that no use of the passed "local" namespace actually happens: the user-supplied code executes with proc->globals as its global namespace and some transient dict created by the function call action as a local namespace. This seems like a very Rube-Goldbergian way of setting up a local namespace for the user-defined code. I think perhaps what we should do is: 1. Compile the user-supplied code directly into a code object, without wrapping it in a "def". (Hence, PLy_procedure_munge_source goes away entirely, which would be nice.) Forget about generating a code object containing a call, too. 2. During function call startup, create a dict to be the local namespace for that call. Shove the argument values into entries in that dict, not the global one. 3. Run the user-supplied code directly with PyEval_EvalCode, passing proc->globals as its global namespace and the transient dict as its local namespace. This would fix the problem with recursive calls and probably be a lot cleaner as well. It would not be 100% backwards compatible, because the technique shown in http://www.postgresql.org/docs/devel/static/plpython-funcs.html of declaring an argument "global" would not work, nor be necessary, anymore. That does not seem like a bad thing, but it would be a reason not to back-patch. Thoughts? regards, tom lane
I wrote:
> This seems like a very Rube-Goldbergian way of setting up a local
> namespace for the user-defined code. I think perhaps what we should do
> is:
> 1. Compile the user-supplied code directly into a code object, without
> wrapping it in a "def". (Hence, PLy_procedure_munge_source goes away
> entirely, which would be nice.) Forget about generating a code object
> containing a call, too.
After further study, it appears this approach won't work because it
breaks "yield" --- AFAICT, Python only allows "yield" inside a "def".
At this point I think what we need is to find a way of passing the
function parameters honestly, that is, as actual parameters in the
manufactured call. I've not looked into how that might be done.
regards, tom lane
On 10/16/2015 10:03 PM, Tom Lane wrote: > I wrote: >> This seems like a very Rube-Goldbergian way of setting up a local >> namespace for the user-defined code. I think perhaps what we should do >> is: >> 1. Compile the user-supplied code directly into a code object, without >> wrapping it in a "def". (Hence, PLy_procedure_munge_source goes away >> entirely, which would be nice.) Forget about generating a code object >> containing a call, too. > After further study, it appears this approach won't work because it > breaks "yield" --- AFAICT, Python only allows "yield" inside a "def". > > At this point I think what we need is to find a way of passing the > function parameters honestly, that is, as actual parameters in the > manufactured call. I've not looked into how that might be done. +1 if it can be done I haven't looked very closely at plpython for a long time, but anything else seems ugly. cheers andrew