Hi Heikki, Thanks for taking a look!
On Sun, Mar 22, 2026 at 5:40 AM Heikki Linnakangas <[email protected]> wrote: > > On 21/03/2026 22:18, Lukas Fittl wrote: > > > > Whilst working on the stack-based instrumentation patch [0] which adds > > a new resource owner that gets set up during a query's execution, I > > encountered the following challenge: > > > > When allocating memory related to a resource, that is intended to be > > accessed during resource owner cleanup on abort, one cannot use a > > memory context that's below the active portal (e.g. the executor > > context), but must instead chose a longer-lived context, often > > TopMemoryContext. > > Yeah, resources managed by resource owners have their own lifecycle > that's independent of memory contexts. FWIW, I don't think this is clearly documented today, so if that's the consensus and we want to keep it that way, we could clarify the resource owner README. > > If we separate out the freeing of this subsidiary portal memory to run > > separately, after resource owner cleanup is done (0001 patch), we can > > remove a handful of uses of TopMemoryContext from the tree in LLVM > > JIT, WaitEventSet and OpenSSL (0002 patch, passes CI), and make it > > much less likely that new resource owner code accidentally leaks > > because it uses the top memory context and missed a pfree. > > It also makes it much more likely that you crash if you release the > resource owner only after deleting the memory context. I don't think > this is a good idea. Do you have a specific case where we intentionally do this today? It seems to me that the consistent coding pattern is that resource owners do get cleaned up before the memory context in the happy path - its just that the abort path has the out-of-order cleanup that occurs with subsidiary memory contexts in a portal. FWIW, from some more research, that early free during abort was added back in 395249ecbeaaf2f2, but it was one of several changes, and its not clear to me if the change intentionally made the memory freed before resource owner cleanup. > I think it's a good rule that anything that's > needed for resource owner cleanup must be allocated in TopMemoryContext, > so that there's no hidden dependency between resource owners and memory > contexts and you can release them in any order. (I'm not 100% sure we > follow that rule everywhere today, but still) It just seems odd to me to require using TopMemoryContext for short-lived memory - especially the fact that it requires doing explicit pfree(..) for every allocation or you leak. > > It also happens to make things significantly easier for the > > stack-based instrumentation patch, since we could rely on the executor > > context to free memory allocations that need to be accessed during > > abort (to propagate instrumentation data up the stack). > > Hmm, I'm not sure I follow. Do you plan to rely on resource owner > cleanup to propagate the instrumentation data? That doesn't feel right > to me. Indeed, that's what's currently being used (feedback certainly welcome), since we don't want to lose instrumentation data during aborts. Using resource owners for this purpose was previously suggested by Andres, and it seems reasonable to me (since they can also handle instrumentation situations outside of the executor), apart from the memory management challenge. The alternative is introducing explicit AtAbort helper functions that get called during Abort(Sub)Transaction. Thanks, Lukas -- Lukas Fittl
