public inbox for [email protected]help / color / mirror / Atom feed
Avoid use of TopMemoryContext for resource owner cleanup in portals 4+ messages / 2 participants [nested] [flat]
* Avoid use of TopMemoryContext for resource owner cleanup in portals @ 2026-03-21 20:18 Lukas Fittl <[email protected]> 2026-03-22 12:39 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Lukas Fittl @ 2026-03-21 20:18 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]>; +Cc: Andres Freund <[email protected]>; Zsolt Parragi <[email protected]> Hi, 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. This is caused by the fact that At(Sub)Abort_Portals currently frees all "subsidiary" memory of failed portals (i.e. failed portal memory child contexts), and will be called in Abort(Sub)Transaction before the ResourceOwnerRelease calls. There appears to be no clear reason why the freeing of subsidiary portal memory is being done before resource owner release - one could argue that freeing memory earlier allows a later allocation to re-use it, but the only relevant case I could find was RecordTransactionAbort, and that is already handled with the pre-allocated TransactionAbortContext to make sure we don't fail allocations in out-of-memory aborts. Other non-portal users of the ResOwner infrastructure don't suffer from this problem, as they typically have a memory context set up that survives the abort. 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 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). Thoughts? Thanks, Lukas [0]: https://www.postgresql.org/message-id/flat/CAP53PkzdBK8VJ1fS4AZ481LgMN8f9mJiC39ZRHqkFUSYq6KWmg%40mai... -- Lukas Fittl Attachments: [application/octet-stream] v1-0002-Stop-using-TopMemoryContext-for-resource-owner-re.patch (6.2K, 2-v1-0002-Stop-using-TopMemoryContext-for-resource-owner-re.patch) download | inline diff: From 29595315f01071bd505cf5d1c94855474f5facbe Mon Sep 17 00:00:00 2001 From: Lukas Fittl <[email protected]> Date: Sat, 21 Mar 2026 11:30:20 -0700 Subject: [PATCH v1 2/2] Stop using TopMemoryContext for resource owner related allocations This removes the unnecessary use of TopMemoryContext for objects that need to be accessed during resource cleanup, instead allocating them in the current memory context. This is made possible thanks to the recent refactoring that delayed freeing portal subsidiary memory until after ResourceOwnerRelease has completed. The current memory context is assumed to be a child context of the currently active portal (if any), like the executor context, or another kind of local context that survives until after res owner cleanup. Author: Lukas Fittl <[email protected]> Reviewed By: Discussion: --- contrib/pgcrypto/openssl.c | 6 ++---- src/backend/jit/llvm/llvmjit.c | 14 +++++++++----- src/backend/storage/ipc/waiteventset.c | 9 +++++++-- src/common/cryptohash_openssl.c | 7 +++---- src/common/hmac_openssl.c | 7 +++---- 5 files changed, 24 insertions(+), 19 deletions(-) diff --git a/contrib/pgcrypto/openssl.c b/contrib/pgcrypto/openssl.c index d3c12e7fda3..f01c105dddf 100644 --- a/contrib/pgcrypto/openssl.c +++ b/contrib/pgcrypto/openssl.c @@ -176,7 +176,7 @@ px_find_digest(const char *name, PX_MD **res) * The order is crucial, to make sure we don't leak anything on * out-of-memory or other error. */ - digest = MemoryContextAlloc(TopMemoryContext, sizeof(*digest)); + digest = palloc(sizeof(*digest)); ctx = EVP_MD_CTX_create(); if (!ctx) @@ -196,7 +196,6 @@ px_find_digest(const char *name, PX_MD **res) digest->owner = CurrentResourceOwner; ResourceOwnerRememberOSSLDigest(digest->owner, digest); - /* The PX_MD object is allocated in the current memory context. */ h = palloc_object(PX_MD); h->result_size = digest_result_size; h->block_size = digest_block_size; @@ -794,7 +793,7 @@ px_find_cipher(const char *name, PX_Cipher **res) * The order is crucial, to make sure we don't leak anything on * out-of-memory or other error. */ - od = MemoryContextAllocZero(TopMemoryContext, sizeof(*od)); + od = palloc0(sizeof(*od)); od->ciph = i->ciph; /* Allocate an EVP_CIPHER_CTX object. */ @@ -812,7 +811,6 @@ px_find_cipher(const char *name, PX_Cipher **res) if (i->ciph->cipher_func) od->evp_ciph = i->ciph->cipher_func(); - /* The PX_Cipher is allocated in current memory context */ c = palloc_object(PX_Cipher); c->block_size = gen_ossl_block_size; c->key_size = gen_ossl_key_size; diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c index 2e8aa4749db..acffd7235bb 100644 --- a/src/backend/jit/llvm/llvmjit.c +++ b/src/backend/jit/llvm/llvmjit.c @@ -233,8 +233,7 @@ llvm_create_context(int jitFlags) ResourceOwnerEnlarge(CurrentResourceOwner); - context = MemoryContextAllocZero(TopMemoryContext, - sizeof(LLVMJitContext)); + context = palloc0(sizeof(LLVMJitContext)); context->base.flags = jitFlags; /* ensure cleanup */ @@ -760,8 +759,13 @@ llvm_compile_module(LLVMJitContext *context) pfree(filename); } + /* + * Allocate handle in the same long-lived context as the LLVMJitContext, + * since this can be called during expression evaluation in a short-lived + * per-tuple context. + */ handle = (LLVMJitHandle *) - MemoryContextAlloc(TopMemoryContext, sizeof(LLVMJitHandle)); + MemoryContextAlloc(GetMemoryChunkContext(context), sizeof(LLVMJitHandle)); /* * Emit the code. Note that this can, depending on the optimization @@ -805,8 +809,8 @@ llvm_compile_module(LLVMJitContext *context) context->module = NULL; context->compiled = true; - /* remember emitted code for cleanup and lookups */ - oldcontext = MemoryContextSwitchTo(TopMemoryContext); + /* remember emitted code for cleanup and lookups. */ + oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(context)); context->handles = lappend(context->handles, handle); MemoryContextSwitchTo(oldcontext); diff --git a/src/backend/storage/ipc/waiteventset.c b/src/backend/storage/ipc/waiteventset.c index 0f228e1e7b8..414e9805eeb 100644 --- a/src/backend/storage/ipc/waiteventset.c +++ b/src/backend/storage/ipc/waiteventset.c @@ -389,9 +389,14 @@ CreateWaitEventSet(ResourceOwner resowner, int nevents) #endif if (resowner != NULL) + { ResourceOwnerEnlarge(resowner); - - data = (char *) MemoryContextAllocZero(TopMemoryContext, sz); + data = (char *) palloc0(sz); + } + else + { + data = (char *) MemoryContextAllocZero(TopMemoryContext, sz); + } set = (WaitEventSet *) data; data += MAXALIGN(sizeof(WaitEventSet)); diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c index 51b7e040933..2373699aa75 100644 --- a/src/common/cryptohash_openssl.c +++ b/src/common/cryptohash_openssl.c @@ -34,12 +34,11 @@ #endif /* - * In the backend, use an allocation in TopMemoryContext to count for - * resowner cleanup handling. In the frontend, use malloc to be able - * to return a failure status back to the caller. + * In backends, use an allocation in the current memory context. In frontend, + * use malloc to be able to return a failure status back to the caller. */ #ifndef FRONTEND -#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size) +#define ALLOC(size) palloc(size) #define FREE(ptr) pfree(ptr) #else #define ALLOC(size) malloc(size) diff --git a/src/common/hmac_openssl.c b/src/common/hmac_openssl.c index 7990822854c..97a1b0756dd 100644 --- a/src/common/hmac_openssl.c +++ b/src/common/hmac_openssl.c @@ -34,13 +34,12 @@ #endif /* - * In backend, use an allocation in TopMemoryContext to count for resowner - * cleanup handling if necessary. In frontend, use malloc to be able to return - * a failure status back to the caller. + * In backends, use an allocation in the current memory context. In frontend, + * use malloc to be able to return a failure status back to the caller. */ #ifndef FRONTEND #define USE_RESOWNER_FOR_HMAC -#define ALLOC(size) MemoryContextAlloc(TopMemoryContext, size) +#define ALLOC(size) palloc(size) #define FREE(ptr) pfree(ptr) #else /* FRONTEND */ #define ALLOC(size) malloc(size) -- 2.47.1 [application/octet-stream] v1-0001-Allow-resource-owners-to-access-subsidiary-portal.patch (7.4K, 3-v1-0001-Allow-resource-owners-to-access-subsidiary-portal.patch) download | inline diff: From 6473d3e1636fb3fbb3f28d118b9b50cd11168d3f Mon Sep 17 00:00:00 2001 From: Lukas Fittl <[email protected]> Date: Sat, 21 Mar 2026 11:02:37 -0700 Subject: [PATCH v1 1/2] Allow resource owners to access subsidiary portal memory during cleanup During AbortTransaction/AbortSubTransaction we free portal child memory contexts, aka subsidiary contexts, ahead of freeing the portal memory itself, in order to not keep short-lived contexts like the executor context around longer than needed, e.g. in the case of holdable cursors. However, we currently free such memory when we first mark the portal as aborted, and before calling ResourceOwnerRelease. This presents a challenge for any resource owner that wants to access memory that was allocated inside the portal (e.g. with the executor context) and is registered to a resource owner that wants to use it during clean up. The current workaround is to allocate such memory in the TopMemoryContext, and use precise pfree(..) calls in both abort and success cases to avoid leaks. Instead, separate marking portals as aborted (and calling the cleanup hook) from releasing subsidiary memory contexts. Delay releasing the memory contexts until ResourceOwnerRelease has been called for all phases. For example, this allows allocating memory in the per-resource context and then accessing it when cleaning up a resource. Since abort handling is not supposed to allocate new memory (the pre-allocated TransactionAbortContext exists for that purpose), this should have limited performance impact in practice, and will allow both potential refactorings of current resource owners, and avoid complexity in future patches. Author: Lukas Fittl <[email protected]> Reviewed By: Discussion: --- src/backend/access/transam/xact.c | 24 ++++++++++++ src/backend/utils/mmgr/portalmem.c | 60 ++++++++++++++++++++++++++++-- src/include/utils/portal.h | 2 + 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index aafc53e0164..3fd3532d452 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -3000,6 +3000,15 @@ AbortTransaction(void) ResourceOwnerRelease(TopTransactionResourceOwner, RESOURCE_RELEASE_AFTER_LOCKS, false, true); + + /* + * Now that resource owner cleanup has finished, we can release the + * memory resource owners might have still accessed during cleanup. + * + * Note the portal context itself is freed later in PortalDrop. + */ + AtAbort_Portals_ReleaseMemory(); + smgrDoPendingDeletes(false); AtEOXact_GUC(false, 1); @@ -3017,6 +3026,10 @@ AbortTransaction(void) AtEOXact_LogicalCtl(); pgstat_report_xact_timestamp(0); } + else + { + AtAbort_Portals_ReleaseMemory(); + } /* * State remains TRANS_ABORT until CleanupTransaction(). @@ -4939,6 +4952,7 @@ AbortOutOfAnyTransaction(void) * we need to shut down before doing CleanupTransaction. */ AtAbort_Portals(); + AtAbort_Portals_ReleaseMemory(); CleanupTransaction(); s->blockState = TBLOCK_DEFAULT; break; @@ -4968,6 +4982,7 @@ AbortOutOfAnyTransaction(void) s->parent->subTransactionId, s->curTransactionOwner, s->parent->curTransactionOwner); + AtSubAbort_Portals_ReleaseMemory(s->subTransactionId); } CleanupSubTransaction(); s = CurrentTransactionState; /* changed by pop */ @@ -5371,6 +5386,15 @@ AbortSubTransaction(void) ResourceOwnerRelease(s->curTransactionOwner, RESOURCE_RELEASE_AFTER_LOCKS, false, false); + + /* + * Now that resource owner cleanup has finished, we can release the + * memory resource owners might have still accessed during cleanup. + * + * Note the portal context itself is freed later in PortalDrop. + */ + AtSubAbort_Portals_ReleaseMemory(s->subTransactionId); + AtSubAbort_smgr(); AtEOXact_GUC(false, s->gucNestLevel); diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c index 493f9b0ee19..d46c42762c8 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -839,12 +839,40 @@ AtAbort_Portals(void) * PortalDrop. */ portal->resowner = NULL; + } +} + +/* + * Release subsidiary memory for aborted portals. + * + * This is called after ResourceOwnerRelease, so that any resources still + * referencing portal memory have been cleaned up first. + */ +void +AtAbort_Portals_ReleaseMemory(void) +{ + HASH_SEQ_STATUS status; + PortalHashEnt *hentry; + + hash_seq_init(&status, PortalHashTable); + + while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL) + { + Portal portal = hentry->portal; + + /* Do nothing to cursors held over from a previous transaction. */ + if (portal->createSubid == InvalidSubTransactionId) + continue; + + /* Do nothing to auto-held cursors. */ + if (portal->autoHeld) + continue; /* * Although we can't delete the portal data structure proper, we can * release any memory in subsidiary contexts, such as executor state. - * The cleanup hook was the last thing that might have needed data - * there. But leave active portals alone. + * The portal cleanup hook or ResourceOwnerRelease were the last thing + * that might have needed data there. But leave active portals alone. */ if (portal->status != PORTAL_ACTIVE) MemoryContextDeleteChildren(portal->portalContext); @@ -1074,11 +1102,35 @@ AtSubAbort_Portals(SubTransactionId mySubid, */ portal->resowner = NULL; + } +} + +/* + * Release subsidiary memory for portals aborted in a subtransaction. + * + * This is called after ResourceOwnerRelease, so that any resources still + * referencing portal memory have been cleaned up first. + */ +void +AtSubAbort_Portals_ReleaseMemory(SubTransactionId mySubid) +{ + HASH_SEQ_STATUS status; + PortalHashEnt *hentry; + + hash_seq_init(&status, PortalHashTable); + + while ((hentry = (PortalHashEnt *) hash_seq_search(&status)) != NULL) + { + Portal portal = hentry->portal; + + if (portal->createSubid != mySubid) + continue; + /* * Although we can't delete the portal data structure proper, we can * release any memory in subsidiary contexts, such as executor state. - * The cleanup hook was the last thing that might have needed data - * there. + * The portal cleanup hook or ResourceOwnerRelease were the last thing + * that might have needed data there. */ MemoryContextDeleteChildren(portal->portalContext); } diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h index a7bedb12c18..41329e8eb24 100644 --- a/src/include/utils/portal.h +++ b/src/include/utils/portal.h @@ -215,6 +215,7 @@ typedef struct PortalData extern void EnablePortalManager(void); extern bool PreCommit_Portals(bool isPrepare); extern void AtAbort_Portals(void); +extern void AtAbort_Portals_ReleaseMemory(void); extern void AtCleanup_Portals(void); extern void PortalErrorCleanup(void); extern void AtSubCommit_Portals(SubTransactionId mySubid, @@ -225,6 +226,7 @@ extern void AtSubAbort_Portals(SubTransactionId mySubid, SubTransactionId parentSubid, ResourceOwner myXactOwner, ResourceOwner parentXactOwner); +extern void AtSubAbort_Portals_ReleaseMemory(SubTransactionId mySubid); extern void AtSubCleanup_Portals(SubTransactionId mySubid); extern Portal CreatePortal(const char *name, bool allowDup, bool dupSilent); extern Portal CreateNewPortal(void); -- 2.47.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Avoid use of TopMemoryContext for resource owner cleanup in portals 2026-03-21 20:18 Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> @ 2026-03-22 12:39 ` Heikki Linnakangas <[email protected]> 2026-03-22 15:23 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Heikki Linnakangas @ 2026-03-22 12:39 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Andres Freund <[email protected]>; Zsolt Parragi <[email protected]> On 21/03/2026 22:18, Lukas Fittl wrote: > Hi, > > 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. > This is caused by the fact that At(Sub)Abort_Portals currently frees > all "subsidiary" memory of failed portals (i.e. failed portal memory > child contexts), and will be called in Abort(Sub)Transaction before > the ResourceOwnerRelease calls. > > There appears to be no clear reason why the freeing of subsidiary > portal memory is being done before resource owner release - one could > argue that freeing memory earlier allows a later allocation to re-use > it, but the only relevant case I could find was > RecordTransactionAbort, and that is already handled with the > pre-allocated TransactionAbortContext to make sure we don't fail > allocations in out-of-memory aborts. > > Other non-portal users of the ResOwner infrastructure don't suffer > from this problem, as they typically have a memory context set up that > survives the abort. > > 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. 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 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. - Heikki ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Avoid use of TopMemoryContext for resource owner cleanup in portals 2026-03-21 20:18 Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> 2026-03-22 12:39 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Heikki Linnakangas <[email protected]> @ 2026-03-22 15:23 ` Lukas Fittl <[email protected]> 2026-03-23 14:33 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Lukas Fittl @ 2026-03-22 15:23 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Zsolt Parragi <[email protected]> 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 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Avoid use of TopMemoryContext for resource owner cleanup in portals 2026-03-21 20:18 Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> 2026-03-22 12:39 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Heikki Linnakangas <[email protected]> 2026-03-22 15:23 ` Re: Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> @ 2026-03-23 14:33 ` Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Heikki Linnakangas @ 2026-03-23 14:33 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Zsolt Parragi <[email protected]> On 22/03/2026 17:23, Lukas Fittl wrote: > 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. +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 ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-03-23 14:33 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-21 20:18 Avoid use of TopMemoryContext for resource owner cleanup in portals Lukas Fittl <[email protected]> 2026-03-22 12:39 ` Heikki Linnakangas <[email protected]> 2026-03-22 15:23 ` Lukas Fittl <[email protected]> 2026-03-23 14:33 ` Heikki Linnakangas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox