Re: memory leak in trigger handling (since PG12)

Hi,

On 2023-05-24 21:49:13 +0200, Tomas Vondra wrote:
> >> 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.

I think architecturally, code storing stuff in a ResultRelInfo - which has
query lifetime - ought to be careful to allocate memory with such lifetime.
Note that the nearby ExecGetRootToChildMap() actually is careful to do so.

> >> 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.

Hm - stepping back a bit, why are we doing the work in ExecGetAllUpdatedCols()
over and over? Unless I am missing something, the result doesn't change
across rows. And it doesn't look that cheap to compute, leaving aside the
allocation that bms_union() does.

It's visible in profiles, not as a top entry, but still.

Perhaps the easiest to backpatch fix is to just avoid recomputing the value?
But perhaps it'd be just as problmeatic, because callers might modify
ExecGetAllUpdatedCols()'s return value in place...

Greetings,

Andres Freund