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. > 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? > 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. Greetings, Andres Freund
