Thread

1. pl/python long-lived allocations in datum->dict transformation

   Jan Urbański <wulczer@wulczer.org> — 2012-02-05T18:54:11Z

   Consider this:
   
   create table arrays as select array[random(), random(), random(),
   random(), random(), random()] as a from generate_series(1, 1000000);
   
   create or replace function plpython_outputfunc() returns void as $$
   c = plpy.cursor('select a from arrays')
   for row in c:
       pass
   $$ language plpythonu;
   
   When running the function, every datum will get transformed into a
   Python dict, which includes calling the type's output function,
   resulting in a memory allocation. The memory is allocated in the SPI
   context, so it accumulates until the function is finished.
   
   This is annoying for functions that plough through large tables, doing
   some calculation. Attached is a patch that does the conversion of
   PostgreSQL Datums into Python dict objects in a scratch memory context
   that gets reset every time.
   
   Cheers,
   Jan
   

2. Re: pl/python long-lived allocations in datum->dict transformation

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-02-11T23:48:23Z

   =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
   > This is annoying for functions that plough through large tables, doing
   > some calculation. Attached is a patch that does the conversion of
   > PostgreSQL Datums into Python dict objects in a scratch memory context
   > that gets reset every time.
   
   As best I can tell, this patch proposes creating a new, separate context
   (chewing up 8KB+) for every plpython procedure that's ever used in a
   given session.  This cure could easily be worse than the disease as far
   as total space consumption is concerned.  What's more, it's unclear that
   it won't malfunction altogether if the function is used recursively
   (ie, what if PLyDict_FromTuple ends up calling the same function again?)
   Can't you fix it so that the temp context is associated with a
   particular function execution, rather than being "statically" allocated
   per-function?
   
   			regards, tom lane
   
   

3. Re: pl/python long-lived allocations in datum->dict transformation

   Jan Urbański <wulczer@wulczer.org> — 2012-02-13T22:40:55Z

   On 12/02/12 00:48, Tom Lane wrote:
   > =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
   >> This is annoying for functions that plough through large tables, doing
   >> some calculation. Attached is a patch that does the conversion of
   >> PostgreSQL Datums into Python dict objects in a scratch memory context
   >> that gets reset every time.
   > 
   > As best I can tell, this patch proposes creating a new, separate context
   > (chewing up 8KB+) for every plpython procedure that's ever used in a
   > given session.  This cure could easily be worse than the disease as far
   
   Yeah, that's not ideal.
   
   > What's more, it's unclear that
   > it won't malfunction altogether if the function is used recursively
   > (ie, what if PLyDict_FromTuple ends up calling the same function again?)
   
   I was a bit worried about that, but the only place where
   PLyDict_FromTuple calls into some other code is when it calls the type's
   specific I/O function, which AFAICT can't call back into user code
   (except for user-defined C I/O routines). It's not very comfortable, but
   I think PLyDict_FromTuple can be allowed to be non-reentrant.
   
   > Can't you fix it so that the temp context is associated with a
   > particular function execution, rather than being "statically" allocated
   > per-function?
   
   That would be cool, but I failed to easily get a handle on something
   that's like the execution context of a PL/Python function... Actually,
   if we assume that PLyDict_FromTuple (which is quite a low-level thing)
   never calls PL/Python UDFs we could keep a single memory context in
   top-level PL/Python memory and pay the overhead once in a session, not
   once per function.
   
   OTOH if we want to make it reentrant, some more tinkering would be in order.
   
   Cheers,
   Jan
   
   

4. Re: pl/python long-lived allocations in datum->dict transformation

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-02-14T00:35:33Z

   =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
   > On 12/02/12 00:48, Tom Lane wrote:
   >> What's more, it's unclear that
   >> it won't malfunction altogether if the function is used recursively
   >> (ie, what if PLyDict_FromTuple ends up calling the same function again?)
   
   > I was a bit worried about that, but the only place where
   > PLyDict_FromTuple calls into some other code is when it calls the type's
   > specific I/O function, which AFAICT can't call back into user code
   > (except for user-defined C I/O routines). It's not very comfortable, but
   > I think PLyDict_FromTuple can be allowed to be non-reentrant.
   
   I think that's pretty short-sighted.  Even if it's safe today (which
   I am not 100% convinced of), there are plenty of foreseeable reasons
   why it might^Wwill break in the future.
   
   * There is no reason to think that datatype I/O functions will never
   be written in anything but C.  People have asked repeatedly for the
   ability to write them in higher-level languages.  I doubt that would
   ever be possible in plpgsql, but with languages that can do
   bit-twiddling like plpython or plperl, it seems possible.
   
   * A datatype I/O function, even if written in C, could call user-written
   code.  See domain_in for example, which can invoke arbitrary processing
   via domain constraint checking.  If you were proposing to patch
   PLyObject_ToTuple rather than the other direction, this patch would be
   breakable today.  Admittedly the breakage would require some rather
   contrived coding ("your domain's constraint check function does
   *what*?"), but it would still be a security bug.
   
   * Once we have the ability to associate a temp memory context with
   plpython, there will be a temptation to use it for other purposes
   besides this one, and it will not be long before such a purpose does
   open a recursion risk, even if there's none there today.  (Speaking of
   which, it sure looks to me like PLyObject_ToDatum, PLyObject_ToTuple,
   etc leak memory like there's no tomorrow.)
   
   > OTOH if we want to make it reentrant, some more tinkering would be in order.
   
   I think that's in order.
   
   			regards, tom lane
   
   

5. Re: pl/python long-lived allocations in datum->dict transformation

   Jan Urbański <wulczer@wulczer.org> — 2012-02-20T00:09:21Z

   On 14/02/12 01:35, Tom Lane wrote:
   > =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
   >> It's not very comfortable, but
   >> I think PLyDict_FromTuple can be allowed to be non-reentrant.
   > 
   > I think that's pretty short-sighted.  Even if it's safe today (which
   > I am not 100% convinced of), there are plenty of foreseeable reasons
   > why it might^Wwill break in the future.
   > 
   >> OTOH if we want to make it reentrant, some more tinkering would be in order.
   > 
   > I think that's in order.
   
   Here are the results of the tinkering.
   
   I came up with a stack of context structures that gets pushed when a
   PL/Python starts being executed and popped when it returns. At first
   they contained just a scratch memory context used by PLyDict_FromTuple.
   Then under the premise of confirming the usefulness of introducing such
   contexts I removed the global PLy_curr_procedure variable and changed
   all users to get the current procedure from the context. It seems to
   have worked, so the total count of global variables is unchanged - hooray!
   
   While testing I found one more leak, this time caused by allocating a
   structure for caching array type I/O functions and never freeing it.
   Attached as separate patch.
   
   Cheers,
   Jan
   

6. Re: pl/python long-lived allocations in datum->dict transformation

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-03-13T17:32:20Z

   =?UTF-8?B?SmFuIFVyYmHFhHNraQ==?= <wulczer@wulczer.org> writes:
   > I came up with a stack of context structures that gets pushed when a
   > PL/Python starts being executed and popped when it returns. At first
   > they contained just a scratch memory context used by PLyDict_FromTuple.
   > Then under the premise of confirming the usefulness of introducing such
   > contexts I removed the global PLy_curr_procedure variable and changed
   > all users to get the current procedure from the context. It seems to
   > have worked, so the total count of global variables is unchanged - hooray!
   
   Applied with some adjustments --- mainly, I thought you were being
   too incautious about ensuring that the stack got popped once it'd been
   pushed.  The easiest way to fix that was to do the pushes after the
   SPI_connect calls, which required decoupling the behavior from
   CurrentMemoryContext, which seemed like a good idea anyway.
   
   > While testing I found one more leak, this time caused by allocating a
   > structure for caching array type I/O functions and never freeing it.
   > Attached as separate patch.
   
   Applied also, but surely if we're leaking memory from the input
   descriptors then we should worry about the output ones too?
   I made it do that, but if that's wrong, somebody explain why.
   
   			regards, tom lane