Re: Memory leak with PL/Python trigger

On Thu, Aug 20, 2015 at 6:18 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> writes:
>> Here I attached updated patch with a context allocation in
>> "PLy_procedure_create" function and the same context is reset
>> in every function call of "PLy_procedure_get" for all PLY types.
>
> This isn't really going in the direction I had in mind.
>
> I think what we want is to get rid of *all* of plpython's retail
> allocations in TopMemoryContext. All the long-lived data about
> a given procedure, starting with its PLyProcedure struct and
> certainly including all the associated PLyTypeInfo stuff, ought
> to be in a per-procedure context, similarly to the way plpgsql
> manages it. Then you can just delete that context if the procedure
> definition changes, and not need any retail cleanup.
>
> Also, you can't just reset the context being used as fn_mcxt
> without any regard for the possibility that functions have
> stored pointers into that space (probably in their fn_extra).
> Really you ought to redo fmgr_info() if you do that. That
> means that this approach is fundamentally giving up the ability
> to cache type lookup data across calls at all, which is surely
> not what we want.

I created a memory context in PLy_procedure_create function and all
the allocations
further on related to PLy_procedure are allocated from that new context. This
context is freed whenever the procedure gets deleted.

Similar changes are done for PLyPlanObject and PLyCursorObject objects.

PLy_malloc0 and PLy_strdup function usage is completely removed
by replacing them with palloc0 and pstrdup. PLy_malloc function usage
is reduced.

> I would envision the PLyTypeInfo structs as usually living in the
> per-procedure context, and that's where fn_mcxt would point as well.
> There might be some cases where we have to have shorter-lived
> PLyTypeInfos, but the normal case ought to work like that.

For the temporary PLyTypeInfos, a temporary memory context is allocated
and used. Instead of using a CurrentMemoryContext or PLyExecutionContext,
I used a new temporary context. The same is freed at the end of the function.

Is the attached patch is in the right direction to the handle the problem?

Regards,
Hari Babu
Fujitsu Australia