public inbox for [email protected]
help / color / mirror / Atom feedFrom: Lukas Fittl <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Zsolt Parragi <[email protected]>
Subject: Avoid use of TopMemoryContext for resource owner cleanup in portals
Date: Sat, 21 Mar 2026 13:18:23 -0700
Message-ID: <CAP53PkxfZu+WgeAC+1BybrS6asH+S_D1Nk5onq7Js5YDFZraJQ@mail.gmail.com> (raw)
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
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Avoid use of TopMemoryContext for resource owner cleanup in portals
In-Reply-To: <CAP53PkxfZu+WgeAC+1BybrS6asH+S_D1Nk5onq7Js5YDFZraJQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox