Re: memory leak in trigger handling (since PG12)

On 5/24/23 21:49, Tomas Vondra wrote:
>
>
> On 5/24/23 20:55, Andres Freund wrote:
>> Hi,
>>
>> On 2023-05-24 15:38:41 +0200, Tomas Vondra wrote:
>>> I looked at this again, and I think GetPerTupleMemoryContext(estate)
>>> might do the trick, see the 0002 part.
>>
>> Yea, that seems like the right thing here.
>>
>>
>>> 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);
>>
>> We could add a variant of ExecGetAllUpdatedCols that switches the context.
>>
>
> Yeah, we could do that. I was thinking about backpatching, and modifying
> ExecGetAllUpdatedCols signature would be ABI break, but adding a
> variant should be fine.
>
>>
>>> 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).
>>
>> Hm - on a quick look the allocations in trigger.c itself are done with
>> MemoryContextAlloc().
>>
>> I did find a problematic path, namely that ExecGetChildToRootMap() ends up
>> building resultRelInfo->ri_ChildToRootMap in CurrentMemoryContext.
>>
>> That seems like a flat out bug to me - we can't just store data in a
>> ResultRelInfo without ensuring the memory is lives long enough. Nearby places
>> like ExecGetRootToChildMap() do make sure to switch to es_query_cxt.
>>
>>
>> Did you see other bits of memory getting allocated in CurrentMemoryContext?
>>
>
> No, I simply tried to do the context switch and then gave up when it
> crashed on the ExecGetRootToChildMap info. I haven't looked much
> further, but you may be right it's the only bit.
>
> It didn't occur to me it might be a bug - I think the code simply
> assumes it gets called with suitable memory context, just like we do in
> various other places. Maybe it should document the assumption.
>
>>
>>> So we'd have to do another switch again. Not sure how
>>> backpatch-friendly would that be.
>>
>> Yea, that's a valid concern. I think it might be reasonable to use something
>> like ExecGetAllUpdatedColsCtx() in the backbranches, and switch to a
>> short-lived context for the trigger invocations in >= 16.
>>
>

The attached patch does this - I realized we actually have estate in
ExecGetAllUpdatedCols(), so we don't even need a variant with a
different signature. That makes the patch much simpler.

The question is whether we need the signature anyway. There might be a
caller expecting the result to be in CurrentMemoryContext (be it
ExecutorState or something else), and this change might break it. But
I'm not aware of any callers, so maybe that's fine.

regards

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