Add assertion for failed alloc to palloc0() and palloc_extended()
Hi,
I noticed that we have Assert(ret != NULL) in palloc() but not in
palloc0() so for consistency I decided to add it. I also added an
assertion that the MCXT_ALLOC_NO_OOM flag is set if alloc() returns
NULL to palloc_extended().
I feel that this might be useful since while palloc() is much more
common the OOM which causes alloc() to incorrectly return NULL could in
theory happen in any of the three functions.
Andreas
Attachments:
0001-Add-assertion-for-failed-alloc-to-palloc0-and-palloc.patchtext/x-patch; charset=UTF-8; name=0001-Add-assertion-for-failed-alloc-to-palloc0-and-palloc.patchDownload
From 7165657a4b62ac35e2c67b9e51035be2a5fbb93f Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <andreas.karlsson@percona.com>
Date: Sat, 1 Mar 2025 00:27:52 +0100
Subject: [PATCH] Add assertion for failed alloc to palloc0() and
palloc_extended()
The palloc() call asserts that the alloc function does not return NULL
so do the same in palloc0(). In palloc_extend() we can assert the same
as long as the MCXT_ALLOC_NO_OOM flag is not set, so let's do so.
---
src/backend/utils/mmgr/mcxt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index aa6da0d0352..ce108788259 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -1356,7 +1356,8 @@ palloc0(Size size)
context->isReset = false;
ret = context->methods->alloc(context, size, 0);
-
+ /* We expect OOM to be handled by the alloc function */
+ Assert(ret != NULL);
VALGRIND_MEMPOOL_ALLOC(context, ret, size);
MemSetAligned(ret, 0, size);
@@ -1379,6 +1380,7 @@ palloc_extended(Size size, int flags)
ret = context->methods->alloc(context, size, flags);
if (unlikely(ret == NULL))
{
+ Assert(flags & MCXT_ALLOC_NO_OOM);
return NULL;
}
--
2.47.2
On Sat, Mar 01, 2025 at 01:05:43AM +0100, Andreas Karlsson wrote:
I noticed that we have Assert(ret != NULL) in palloc() but not in palloc0()
so for consistency I decided to add it. I also added an assertion that the
MCXT_ALLOC_NO_OOM flag is set if alloc() returns
NULL to palloc_extended().I feel that this might be useful since while palloc() is much more common
the OOM which causes alloc() to incorrectly return NULL could in theory
happen in any of the three functions.
Hmm. Good points. All the MemoryContextMethods rely on
MemoryContextAllocationFailure() to handle the case of
MCXT_ALLOC_NO_OOM on failure (except alignedalloc which has no alloc
routine). Your two suggestions, one in palloc0() for the non-NULL
check, and the second one in palloc_extended() to make sure that we
have MCXT_ALLOC_NO_OOM set when the result is NULL, could catch
inconsistencies when implementing a new method.
In short, LGTM. Will apply if there are no objections.
--
Michael
On Mon, Mar 03, 2025 at 01:13:05PM +0900, Michael Paquier wrote:
In short, LGTM. Will apply if there are no objections.
And applied as 40d3f8274499.
--
Michael