Re: memory leak in trigger handling (since PG12)

On 5/23/23 22:57, Tomas Vondra wrote:
> 
> 
> On 5/23/23 18:39, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>>> it seems there's a fairly annoying memory leak in trigger code,
>>> introduced by
>>> ...
>>> Attached is a patch, restoring the pre-12 behavior for me.
>>
>>> While looking for other places allocating stuff in ExecutorState (for
>>> the UPDATE case) and leaving it there, I found two more cases:
>>
>>> 1) copy_plpgsql_datums
>>
>>> 2) make_expanded_record_from_tupdesc
>>>    make_expanded_record_from_exprecord
>>
>>> All of this is calls from plpgsql_exec_trigger.
>>
>> Not sure about the expanded-record case, but both of your other two
>> fixes feel like poor substitutes for pushing the memory into a
>> shorter-lived context.  In particular I'm quite surprised that
>> plpgsql isn't already allocating that workspace in the "procedure"
>> memory context.
>>
> 
> I don't disagree, but which memory context should this use and
> when/where should we switch to it?
> 
> I haven't seen any obvious memory context candidate in the code
> calling ExecGetAllUpdatedCols, so I guess we'd have to pass it from
> above. Is that a good idea for backbranches ...
> 

I looked at this again, and I think GetPerTupleMemoryContext(estate)
might do the trick, see the 0002 part. Unfortunately it's not much
smaller/simpler than just freeing the chunks, because we end up doing

    oldcxt = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
    updatedCols = ExecGetAllUpdatedCols(relinfo, estate);
    MemoryContextSwitchTo(oldcxt);

and then have to pass updatedCols elsewhere. It's tricky to just switch
to the context (e.g. in ExecASUpdateTriggers/ExecARUpdateTriggers), as
AfterTriggerSaveEvent allocates other bits of memory too (in a longer
lived context). So we'd have to do another switch again. Not sure how
backpatch-friendly would that be.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company