Re: Avoid use of TopMemoryContext for resource owner cleanup in portals - Mailing list pgsql-hackers

On 22/03/2026 17:23, Lukas Fittl wrote:
> On Sun, Mar 22, 2026 at 5:40 AM Heikki Linnakangas <hlinnaka@iki.fi> 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.

+1

>>> 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 don't know if we intentionally cleanup memory vs resource owners in 
any particular order today, but life is easier if you don't have to do 
things in a particular order.

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

For more complicated data structures associated with a resource tracked 
by a ResourceOwner, I'd recommend creating a dedicated memory context 
with TopMemoryContext as its parent.

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

I'll take a look at that thread and chime in there..

- Heikki