Re: [HACKERS] Safe memory allocation functions
On Sat, Jan 31, 2015 at 2:58 AM, Robert Haas robertmh...@gmail.com wrote: Committed. I didn't think we really need to expose two separate flags for the aligned and unaligned cases, so I ripped that out. I also removed the duplicate documentation of the new constants in the function header; having two copies of the documentation, one far removed from the constants themselves, is a recipe for them eventually getting out of sync. I also moved the function to what I thought was a more logical place in the file, and rearranged the order of tests slightly so that, in the common path, we test only whether ret == NULL and not anything else. Thanks a lot! I have reworked a bit the module used for the tests of the new extended routine (with new tests for Alloc, AllocZero and AllocHuge as well) and pushed it here: https://github.com/michaelpq/pg_plugins/tree/master/mcxtalloc_test This is just to mention it for the sake of the archives, I cannot really believe that it should be an in-core module in src/test/modules as it does allocations of 1GB in environments where there is enough memory. Note that it contains regression tests with alternate outputs depending on the environment where they are run (still output may not be stable depending on where those tests are run, particularly where memory usage varies a lot). -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Fri, Jan 30, 2015 at 1:10 AM, Michael Paquier michael.paqu...@gmail.com wrote: I wrote: Yes, this refactoring was good for testing actually... Oops, I have been too hasty when sending previous patch, there was a bug related to huge allocations. Patch correcting this bug is attached. Committed. I didn't think we really need to expose two separate flags for the aligned and unaligned cases, so I ripped that out. I also removed the duplicate documentation of the new constants in the function header; having two copies of the documentation, one far removed from the constants themselves, is a recipe for them eventually getting out of sync. I also moved the function to what I thought was a more logical place in the file, and rearranged the order of tests slightly so that, in the common path, we test only whether ret == NULL and not anything else. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Wed, Jan 28, 2015 at 9:34 AM, Michael Paquier michael.paqu...@gmail.com wrote: As a result of all the comments on this thread, here are 3 patches implementing incrementally the different ideas from everybody: 1) 0001 modifies aset.c to return unconditionally NULL in case of an OOM instead of reporting an error. All the OOM error reports are moved to mcxt.c (MemoryContextAlloc* and palloc*) This seems like a good idea, but I think it's pretty clear that the MemoryContextStats(TopMemoryContext) calls ought to move along with the OOM error report. The stats are basically another kind of error-case output, and the whole point here is that the caller wants to have control of what happens when malloc fails. Committed that way. 2) 0002 adds the noerror routines for frontend and backend. We don't have consensus on this name; as I read it, Andres and I are both strongly opposed to it. Instead of continuing to litigate that point, I'd like to propose that we just leave this out. We are unlikely to have so many callers who need the no-oom-error behavior to justify adding a bunch of convenience functions --- and if that does happen, we can resume arguing about the naming then. For now, let's just say that if you want that behavior, you should use MemoryContextAllocExtended(CurrentMemoryContext, ...). 3) 0003 adds MemoryContextAllocExtended that can be called with the following control flags: #define ALLOC_HUGE 0x01/* huge allocation */ #define ALLOC_ZERO 0x02/* clear allocated memory */ #define ALLOC_NO_OOM 0x04/* no failure if out-of-memory */ #define ALLOC_ALIGNED 0x08/* request length suitable for MemSetLoop */ This groups MemoryContextAlloc, MemoryContextAllocHuge, MemoryContextAllocZero and MemoryContextAllocZeroAligned under the same central routine. I recommend we leave the existing MemoryContextAlloc* functions alone and add a new MemoryContextAllocExtended() function *in addition*. I think the reason we have multiple copies of this code is because they are sufficiently hot to make the effect of a few extra CPU instructions potentially material. By having separate copies of the code, we avoid introducing extra branches. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
I wrote:
Yes, this refactoring was good for testing actually...
Oops, I have been too hasty when sending previous patch, there was a
bug related to huge allocations. Patch correcting this bug is
attached.
Attached are as well two things I have used to test the new API:
- A hack refactoring the existing routines MemoryContextAlloc* to use
the extended API
- An extension with a function doing a direct call to the extended API
able to control the flags used:
CREATE FUNCTION blackhole_palloc(size bigint,
is_huge bool,
is_no_oom bool,
is_zero bool,
is_zero_aligned bool)
Here are some tests done on a small box of 384MB with direct calls of
the extended API:
=# create extension blackhole ;
CREATE EXTENSION
-- failure for normal allocation because size = 1GB
=# select blackhole_palloc(1024 * 1024 * 1024, false, false, false, false);
ERROR: XX000: invalid memory alloc request size 1073741824
LOCATION: MemoryContextAllocExtended, mcxt.c:628
-- Failure of OOM because normal allocation can be done, but no memory
=# select blackhole_palloc(1024 * 1024 * 1024 - 1, false, false, false, false);
ERROR: 53200: out of memory
DETAIL: Failed on request of size 1073741823.
LOCATION: MemoryContextAllocExtended, mcxt.c:639
-- No failure, bypassing OOM error
=# select blackhole_palloc(1024 * 1024 * 1024 - 1, false, true, false, false);
blackhole_palloc
--
null
(1 row)
-- Huge allocation, no error because OOM error is bypassed
=# select blackhole_palloc(1024 * 1024 * 1024, true, true, false, false);
blackhole_palloc
--
null
(1 row)
-- OOM error, huge allocation failure
=# select blackhole_palloc(1024 * 1024 * 1024, true, false, false, false);
ERROR: 53200: out of memory
DETAIL: Failed on request of size 1073741824.
LOCATION: MemoryContextAllocExtended, mcxt.c:639
-- Assertion failure, zero and zero aligned cannot be called at the same time
=# select blackhole_palloc(1024 * 1024, false, false, true, true);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
--
Michael
From 09bf9364a80dfe6426c91bdefe6ae2135e6f20c3 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Fri, 30 Jan 2015 12:56:21 +0900
Subject: [PATCH 1/2] Create MemoryContextAllocExtended central routine for
memory allocation
This new routine is the central point can be used by extensions and
third-part utilities in a more extensive way than the already present
routines MemoryContextAlloc, one of the control flags introduced being
particularly useful to avoid out-of-memory errors when allocation request
cannot be completed correctly.
---
src/backend/utils/mmgr/mcxt.c | 57 +++
src/include/utils/palloc.h| 12 +
2 files changed, 69 insertions(+)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index c62922a..fce8a0e 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -603,6 +603,63 @@ MemoryContextCreate(NodeTag tag, Size size,
}
/*
+ * MemoryContextAllocExtended
+ * Allocate space within the specified context using flag options
+ * defined by caller.
+ *
+ * The following control flags can be used:
+ * - MCXT_ALLOC_HUGE, allocate possibly-expansive space. this is
+ * equivalent to MemoryContextAllocHuge.
+ * - MCXT_ALLOC_NO_OOM, not fail in case of allocation request
+ * failure and return NULL.
+ * - MCXT_ALLOC_ZERO, clear allocated memory using MemSetAligned.
+ * - MCXT_ALLOC_ZERO_ALIGNED, clear memory using MemSetLoop.
+ */
+void *
+MemoryContextAllocExtended(MemoryContext context, Size size, int flags)
+{
+ void *ret;
+
+ AssertArg(MemoryContextIsValid(context));
+ AssertNotInCriticalSection(context);
+
+ if (((flags MCXT_ALLOC_HUGE) != 0 !AllocHugeSizeIsValid(size)) ||
+ ((flags MCXT_ALLOC_HUGE) == 0 !AllocSizeIsValid(size)))
+ elog(ERROR, invalid memory alloc request size %zu, size);
+
+ context-isReset = false;
+
+ ret = (*context-methods-alloc) (context, size);
+ if ((flags MCXT_ALLOC_NO_OOM) == 0 ret == NULL)
+ {
+ MemoryContextStats(TopMemoryContext);
+ ereport(ERROR,
+(errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg(out of memory),
+ errdetail(Failed on request of size %zu., size)));
+ }
+
+ if (ret == NULL)
+ return NULL;
+
+ VALGRIND_MEMPOOL_ALLOC(context, ret, size);
+
+ /*
+ * MemSetAligned and MemSetLoop should not be called in the same
+ * context (see c.h for more details).
+ */
+ Assert((flags MCXT_ALLOC_ZERO) == 0 ||
+ (flags MCXT_ALLOC_ZERO_ALIGNED) == 0);
+
+ if ((flags MCXT_ALLOC_ZERO) != 0)
+ MemSetAligned(ret, 0, size);
+ if ((flags MCXT_ALLOC_ZERO_ALIGNED) != 0)
+ MemSetLoop(ret, 0, size);
+
+ return ret;
+}
+
+/*
* MemoryContextAlloc
* Allocate space within the specified context.
*
diff --git a/src/include/utils/palloc.h
Re: [HACKERS] Safe memory allocation functions
On Tue, Jan 27, 2015 at 5:34 PM, Andres Freund and...@2ndquadrant.com wrote:
On 2015-01-27 17:27:53 +0900, Michael Paquier wrote:
Alvaro Herrera wrote:
So how about something like
#define ALLOCFLAG_HUGE 0x01
#define ALLOCFLAG_NO_ERROR_ON_OOM 0x02
void *
MemoryContextAllocFlags(MemoryContext context, Size size, int flags);
The flag for huge allocations may be useful, but I don't actually see
much value in the flag ALLOC_NO_OOM if the stuff in aset.c returns
unconditionally NULL in case of an OOM and we let palloc complain
about an OOM when allocation returns NULL. Something I am missing
perhaps?
I guess the idea is to have *user facing* MemoryContextAllocExtended()
that can do both huge and no-oom allocations. Otherwise we need palloc
like wrappers for all combinations.
We're certainly not just going to ignore memory allocation failures
generally in in MemoryContextAllocExtended()
As a result of all the comments on this thread, here are 3 patches
implementing incrementally the different ideas from everybody:
1) 0001 modifies aset.c to return unconditionally NULL in case of an
OOM instead of reporting an error. All the OOM error reports are moved
to mcxt.c (MemoryContextAlloc* and palloc*)
2) 0002 adds the noerror routines for frontend and backend.
3) 0003 adds MemoryContextAllocExtended that can be called with the
following control flags:
#define ALLOC_HUGE 0x01/* huge allocation */
#define ALLOC_ZERO 0x02/* clear allocated memory */
#define ALLOC_NO_OOM 0x04/* no failure if out-of-memory */
#define ALLOC_ALIGNED 0x08/* request length suitable for MemSetLoop */
This groups MemoryContextAlloc, MemoryContextAllocHuge,
MemoryContextAllocZero and MemoryContextAllocZeroAligned under the
same central routine.
Regards,
--
Michael
From 337c439554ce66486cf9d29dced6c72d034b8f8d Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Wed, 28 Jan 2015 22:10:13 +0900
Subject: [PATCH 1/3] Make allocation return functions return NULL on OOM
On counterpart, higher-level APIs in mcxt.c return an explicit error
message when memory requests cannot be completed.
---
src/backend/utils/mmgr/aset.c | 30 ---
src/backend/utils/mmgr/mcxt.c | 48 +++
2 files changed, 61 insertions(+), 17 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 85b3c9a..bf6f09a 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -642,8 +642,8 @@ AllocSetDelete(MemoryContext context)
/*
* AllocSetAlloc
- * Returns pointer to allocated memory of given size; memory is added
- * to the set.
+ * Returns pointer to allocated memory of given size or NULL if
+ * request could not be completed; memory is added to the set.
*
* No request may exceed:
* MAXALIGN_DOWN(SIZE_MAX) - ALLOC_BLOCKHDRSZ - ALLOC_CHUNKHDRSZ
@@ -673,10 +673,7 @@ AllocSetAlloc(MemoryContext context, Size size)
if (block == NULL)
{
MemoryContextStats(TopMemoryContext);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg(out of memory),
- errdetail(Failed on request of size %zu., size)));
+ return NULL;
}
block-aset = set;
block-freeptr = block-endptr = ((char *) block) + blksize;
@@ -867,10 +864,7 @@ AllocSetAlloc(MemoryContext context, Size size)
if (block == NULL)
{
MemoryContextStats(TopMemoryContext);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg(out of memory),
- errdetail(Failed on request of size %zu., size)));
+ return NULL;
}
block-aset = set;
@@ -1002,9 +996,10 @@ AllocSetFree(MemoryContext context, void *pointer)
/*
* AllocSetRealloc
- * Returns new pointer to allocated memory of given size; this memory
- * is added to the set. Memory associated with given pointer is copied
- * into the new memory, and the old memory is freed.
+ * Returns new pointer to allocated memory of given size or NULL if
+ * request could not be completed; this memory is added to the set.
+ * Memory associated with given pointer is copied into the new memory,
+ * and the old memory is freed.
*
* Without MEMORY_CONTEXT_CHECKING, we don't know the old request size. This
* makes our Valgrind client requests less-precise, hazarding false negatives.
@@ -1109,10 +1104,7 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
if (block == NULL)
{
MemoryContextStats(TopMemoryContext);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg(out of memory),
- errdetail(Failed on request of size %zu., size)));
+ return NULL;
}
block-freeptr = block-endptr = ((char *) block) + blksize;
@@ -1179,6 +1171,10 @@ AllocSetRealloc(MemoryContext context, void *pointer, Size size)
/* allocate new chunk */
newPointer = AllocSetAlloc((MemoryContext) set, size);
+ /* leave immediately if request
Re: [HACKERS] Safe memory allocation functions
Alvaro Herrera wrote: So how about something like #define ALLOCFLAG_HUGE 0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); The flag for huge allocations may be useful, but I don't actually see much value in the flag ALLOC_NO_OOM if the stuff in aset.c returns unconditionally NULL in case of an OOM and we let palloc complain about an OOM when allocation returns NULL. Something I am missing perhaps? I definitely do not want to push the nofail stuff via the MemoryContextData- API into aset.c. Imo aset.c should always return NULL and then mcxt.c should throw the error if in the normal palloc() function. Sure, that seems reasonable ... Yes, this would simplify the footprint of this patch to aset.c to a minimum by changing the ereport to NULL in a couple of places. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On 2015-01-27 17:27:53 +0900, Michael Paquier wrote: Alvaro Herrera wrote: So how about something like #define ALLOCFLAG_HUGE 0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); The flag for huge allocations may be useful, but I don't actually see much value in the flag ALLOC_NO_OOM if the stuff in aset.c returns unconditionally NULL in case of an OOM and we let palloc complain about an OOM when allocation returns NULL. Something I am missing perhaps? I guess the idea is to have *user facing* MemoryContextAllocExtended() that can do both huge and no-oom allocations. Otherwise we need palloc like wrappers for all combinations. We're certainly not just going to ignore memory allocation failures generally in in MemoryContextAllocExtended() Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Sat, Jan 17, 2015 at 11:06 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jan 16, 2015 at 10:56 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: So how about something like #define ALLOCFLAG_HUGE 0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); That sounds good, although personally I'd rather have the name be something like MemoryContextAllocExtended; we have precedent for using Extended for this sort of thing elsewhere. Also, I'd suggest trying to keep the flag name short, e.g. ALLOC_HUGE and ALLOC_NO_OOM (or ALLOC_SOFT_FAIL?). Yes, I think that this name makes more sense (LockAcquire[Extended], RangeVarGetRelid[Extended]), as well as minimizing shorter name for the flags. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Fri, Jan 16, 2015 at 10:56 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: So how about something like #define ALLOCFLAG_HUGE 0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); That sounds good, although personally I'd rather have the name be something like MemoryContextAllocExtended; we have precedent for using Extended for this sort of thing elsewhere. Also, I'd suggest trying to keep the flag name short, e.g. ALLOC_HUGE and ALLOC_NO_OOM (or ALLOC_SOFT_FAIL?). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Fri, Jan 16, 2015 at 8:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
Voting for palloc_noerror() as well.
And here is an updated patch using this naming, added to the next CF as well.
--
Michael
From b636c809c2f2cb4177bedc2e5a4883a79b61fbc6 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Tue, 13 Jan 2015 15:40:38 +0900
Subject: [PATCH] Add memory allocation APIs able to return NULL instead of
ERROR
The following functions are added to the existing set for frontend and
backend:
- palloc_noerror
- palloc0_noerror
- repalloc_noerror
---
src/backend/utils/mmgr/aset.c| 529 +++
src/backend/utils/mmgr/mcxt.c| 124 +
src/common/fe_memutils.c | 72 --
src/include/common/fe_memutils.h | 3 +
src/include/nodes/memnodes.h | 2 +
src/include/utils/palloc.h | 3 +
6 files changed, 451 insertions(+), 282 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 85b3c9a..974e018 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -243,11 +243,22 @@ typedef struct AllocChunkData
((AllocPointer)(((char *)(chk)) + ALLOC_CHUNKHDRSZ))
/*
+ * Wrappers for allocation functions.
+ */
+static void *set_alloc_internal(MemoryContext context,
+Size size, bool noerror);
+static void *set_realloc_internal(MemoryContext context, void *pointer,
+ Size size, bool noerror);
+
+/*
* These functions implement the MemoryContext API for AllocSet contexts.
*/
static void *AllocSetAlloc(MemoryContext context, Size size);
+static void *AllocSetAllocNoError(MemoryContext context, Size size);
static void AllocSetFree(MemoryContext context, void *pointer);
static void *AllocSetRealloc(MemoryContext context, void *pointer, Size size);
+static void *AllocSetReallocNoError(MemoryContext context,
+ void *pointer, Size size);
static void AllocSetInit(MemoryContext context);
static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
@@ -264,8 +275,10 @@ static void AllocSetCheck(MemoryContext context);
*/
static MemoryContextMethods AllocSetMethods = {
AllocSetAlloc,
+ AllocSetAllocNoError,
AllocSetFree,
AllocSetRealloc,
+ AllocSetReallocNoError,
AllocSetInit,
AllocSetReset,
AllocSetDelete,
@@ -517,140 +530,16 @@ AllocSetContextCreate(MemoryContext parent,
}
/*
- * AllocSetInit
- * Context-type-specific initialization routine.
- *
- * This is called by MemoryContextCreate() after setting up the
- * generic MemoryContext fields and before linking the new context
- * into the context tree. We must do whatever is needed to make the
- * new context minimally valid for deletion. We must *not* risk
- * failure --- thus, for example, allocating more memory is not cool.
- * (AllocSetContextCreate can allocate memory when it gets control
- * back, however.)
- */
-static void
-AllocSetInit(MemoryContext context)
-{
- /*
- * Since MemoryContextCreate already zeroed the context node, we don't
- * have to do anything here: it's already OK.
- */
-}
-
-/*
- * AllocSetReset
- * Frees all memory which is allocated in the given set.
- *
- * Actually, this routine has some discretion about what to do.
- * It should mark all allocated chunks freed, but it need not necessarily
- * give back all the resources the set owns. Our actual implementation is
- * that we hang onto any keeper block specified for the set. In this way,
- * we don't thrash malloc() when a context is repeatedly reset after small
- * allocations, which is typical behavior for per-tuple contexts.
- */
-static void
-AllocSetReset(MemoryContext context)
-{
- AllocSet set = (AllocSet) context;
- AllocBlock block;
-
- AssertArg(AllocSetIsValid(set));
-
-#ifdef MEMORY_CONTEXT_CHECKING
- /* Check for corruption and leaks before freeing */
- AllocSetCheck(context);
-#endif
-
- /* Clear chunk freelists */
- MemSetAligned(set-freelist, 0, sizeof(set-freelist));
-
- block = set-blocks;
-
- /* New blocks list is either empty or just the keeper block */
- set-blocks = set-keeper;
-
- while (block != NULL)
- {
- AllocBlock next = block-next;
-
- if (block == set-keeper)
- {
- /* Reset the block, but don't return it to malloc */
- char *datastart = ((char *) block) + ALLOC_BLOCKHDRSZ;
-
-#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(datastart, block-freeptr - datastart);
-#else
- /* wipe_mem() would have done this */
- VALGRIND_MAKE_MEM_NOACCESS(datastart, block-freeptr - datastart);
-#endif
- block-freeptr = datastart;
- block-next = NULL;
- }
- else
- {
- /* Normal case, release the block */
-#ifdef CLOBBER_FREED_MEMORY
- wipe_mem(block, block-freeptr - ((char *) block));
-#endif
- free(block);
- }
- block = next;
- }
-
- /* Reset block size allocation sequence, too */
- set-nextBlockSize = set-initBlockSize;
-}
-
-/*
- * AllocSetDelete
- * Frees all memory which is allocated in the given set,
-
Re: [HACKERS] Safe memory allocation functions
On 2015-01-16 08:47:10 +0900, Michael Paquier wrote: On Fri, Jan 16, 2015 at 12:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. Voting for palloc_noerror() as well. I don't like that name. It very well can error out. E.g. because of the allocation size. And we definitely do not want to ignore that case. How about palloc_try()? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On 2015-01-16 23:06:12 +0900, Michael Paquier wrote:
/*
+ * Wrappers for allocation functions.
+ */
+static void *set_alloc_internal(MemoryContext context,
+ Size size, bool
noerror);
+static void *set_realloc_internal(MemoryContext context, void *pointer,
+ Size size,
bool noerror);
+
+/*
* These functions implement the MemoryContext API for AllocSet contexts.
*/
static void *AllocSetAlloc(MemoryContext context, Size size);
+static void *AllocSetAllocNoError(MemoryContext context, Size size);
static void AllocSetFree(MemoryContext context, void *pointer);
static void *AllocSetRealloc(MemoryContext context, void *pointer, Size
size);
+static void *AllocSetReallocNoError(MemoryContext context,
+ void *pointer,
Size size);
static void AllocSetInit(MemoryContext context);
static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
@@ -264,8 +275,10 @@ static void AllocSetCheck(MemoryContext context);
*/
static MemoryContextMethods AllocSetMethods = {
AllocSetAlloc,
+ AllocSetAllocNoError,
AllocSetFree,
AllocSetRealloc,
+ AllocSetReallocNoError,
AllocSetInit,
AllocSetReset,
AllocSetDelete,
@@ -517,140 +530,16 @@ AllocSetContextCreate(MemoryContext parent,
}
Wouldn't it make more sense to change the MemoryContext API to return
NULLs in case of allocation failure and do the error checking in the
mcxt.c callers?
+/* wrapper routines for allocation */
+static void* palloc_internal(Size size, bool noerror);
+static void* repalloc_internal(void *pointer, Size size, bool noerror);
+
/*
* You should not do memory allocations within a critical section, because
* an out-of-memory error will be escalated to a PANIC. To enforce that
@@ -684,8 +688,8 @@ MemoryContextAllocZeroAligned(MemoryContext context, Size
size)
return ret;
}
-void *
-palloc(Size size)
+static void*
+palloc_internal(Size size, bool noerror)
{
/* duplicates MemoryContextAlloc to avoid increased overhead */
void *ret;
@@ -698,31 +702,85 @@ palloc(Size size)
CurrentMemoryContext-isReset = false;
- ret = (*CurrentMemoryContext-methods-alloc) (CurrentMemoryContext,
size);
+ if (noerror)
+ ret = (*CurrentMemoryContext-methods-alloc_noerror)
+ (CurrentMemoryContext, size);
+ else
+ ret = (*CurrentMemoryContext-methods-alloc)
+ (CurrentMemoryContext, size);
VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
return ret;
}
I'd be rather surprised if these branches won't show up in
profiles. This is really rather hot code. At the very least this helper
function should be inlined. Also, calling the valgrind function on an
allocation failure surely isn't correct.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Hmm, I understood Tom to be opposing the idea of a palloc variant that returns NULL on failure, and I understand you to be supporting it. But maybe I'm confused. Your understanding seems correct to me. I was just saying that your description of Tom's argument to dislike the idea seemed at odds with what he was actually saying. OK, that may be. I'm not sure. Anyway, I support it. I agree that there are systems (or circumstances?) where malloc is going to succeed and then the world will blow up later on anyway, but I don't think that means that an out-of-memory error is the only sensible response to a palloc failure; returning NULL seems like a sometimes-useful alternative. I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. I don't care for noerror() because it probably still will error in some circumstances; just not for OOM. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Robert Haas wrote: On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. I don't care for noerror() because it probably still will error in some circumstances; just not for OOM. Yes, but that seems fine to me. We have other functions with noerror flags, and they can still fail under some circumstances -- just not if the error is the most commonly considered scenario in which they fail. The first example I found is LookupAggNameTypeNames(); there are many more. I don't think this causes any confusion in practice. Another precendent we have is something like missing_ok as a flag name in get_object_address() and other places; following that, we could have this new function as palloc_oom_ok or something like that. But it doesn't seem an improvement to me. (I'm pretty sure we all agree that this must not be a flag to palloc but rather a new function.) Of all the ones you proposed above, the one I like the most is palloc_no_oom, but IMO palloc_noerror is still better. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On 2015-01-16 12:09:25 -0300, Alvaro Herrera wrote: Robert Haas wrote: On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. I don't care for noerror() because it probably still will error in some circumstances; just not for OOM. Yes, but that seems fine to me. We have other functions with noerror flags, and they can still fail under some circumstances -- just not if the error is the most commonly considered scenario in which they fail. We rely on palloc erroring out on large allocations in a couple places as a crosscheck. I don't think this argument holds much water. The first example I found is LookupAggNameTypeNames(); there are many more. I don't think this causes any confusion in practice. Another precendent we have is something like missing_ok as a flag name in get_object_address() and other places; following that, we could have this new function as palloc_oom_ok or something like that. But it doesn't seem an improvement to me. (I'm pretty sure we all agree that this must not be a flag to palloc but rather a new function.) Of all the ones you proposed above, the one I like the most is palloc_no_oom, but IMO palloc_noerror is still better. Neither seem very accurate. no_oom isn't true because they actually can cause ooms. _noerror isn't true because they can error out - we e.g. rely on palloc erroring out when reading toast tuples (to detect invalid datum lengths) and during parsing of WAL as an additional defense. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Andres Freund wrote: On 2015-01-16 12:09:25 -0300, Alvaro Herrera wrote: Robert Haas wrote: On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. I don't care for noerror() because it probably still will error in some circumstances; just not for OOM. Yes, but that seems fine to me. We have other functions with noerror flags, and they can still fail under some circumstances -- just not if the error is the most commonly considered scenario in which they fail. We rely on palloc erroring out on large allocations in a couple places as a crosscheck. I don't think this argument holds much water. I don't understand what that has to do with it. Surely we're not going to have palloc_noerror() not error out when presented with a huge allocation. My point is just that the noerror bit in palloc_noerror() means that it doesn't fail in OOM, and that there are other causes for it to error. One thought I just had is that we also have MemoryContextAllocHuge; are we going to consider a mixture of both things in the future, i.e. allow huge allocations to return NULL when OOM? It sounds a bit useless currently, but it doesn't seem extremely far-fetched that we will need further flags in the future. (Or, perhaps, we will want to have code that retries a Huge allocation that returns NULL with a smaller size, just in case it does work.) Maybe what we need is to turn these things into flags to a new generic function. Furthermore, I question whether we really need a palloc variant -- I mean, can we live with just the MemoryContext API instead? As with the Huge variant (which does not have a corresponding palloc equivalent), possible use cases seem very limited so there's probably not much point in providing a shortcut. So how about something like #define ALLOCFLAG_HUGE 0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); and perhaps even #define MemoryContextAllocHuge(cxt, sz) \ MemoryContextAllocFlags(cxt, sz, ALLOCFLAG_HUGE) for source-level compatibility. (Now we all agree that palloc() itself is a very hot spot and shouldn't be touched at all. I don't think these new functions are used as commonly as that, so the fact that they are slightly slower shouldn't be too troublesome.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On 2015-01-16 12:56:18 -0300, Alvaro Herrera wrote: Andres Freund wrote: We rely on palloc erroring out on large allocations in a couple places as a crosscheck. I don't think this argument holds much water. I don't understand what that has to do with it. Surely we're not going to have palloc_noerror() not error out when presented with a huge allocation. Precisely. That means it *does* error out in a somewhat expected path. My point is just that the noerror bit in palloc_noerror() means that it doesn't fail in OOM, and that there are other causes for it to error. That description pretty much describes why it's a misnomer, no? One thought I just had is that we also have MemoryContextAllocHuge; are we going to consider a mixture of both things in the future, i.e. allow huge allocations to return NULL when OOM? I definitely think we should. I'd even say that the usecase is larger for huge allocations. It'd e.g. be rather nice to first try sorting with the huge 16GB work mem and then try 8GB/4/1GB if that fails. It sounds a bit useless currently, but it doesn't seem extremely far-fetched that we will need further flags in the future. (Or, perhaps, we will want to have code that retries a Huge allocation that returns NULL with a smaller size, just in case it does work.) Maybe what we need is to turn these things into flags to a new generic function. Furthermore, I question whether we really need a palloc variant -- I mean, can we live with just the MemoryContext API instead? As with the Huge variant (which does not have a corresponding palloc equivalent), possible use cases seem very limited so there's probably not much point in providing a shortcut. I'm fine with not providing a palloc() equivalent, but I also am fine with having it. So how about something like #define ALLOCFLAG_HUGE0x01 #define ALLOCFLAG_NO_ERROR_ON_OOM 0x02 void * MemoryContextAllocFlags(MemoryContext context, Size size, int flags); and perhaps even #define MemoryContextAllocHuge(cxt, sz) \ MemoryContextAllocFlags(cxt, sz, ALLOCFLAG_HUGE) for source-level compatibility. I don't know, this seems a bit awkward to use. Your earlier example with the *Huge variant that returns a smaller allocation doesn't really convince me - that'd need a separate API anyway. I definitely do not want to push the nofail stuff via the MemoryContextData- API into aset.c. Imo aset.c should always return NULL and then mcxt.c should throw the error if in the normal palloc() function. (Now we all agree that palloc() itself is a very hot spot and shouldn't be touched at all. I don't think these new functions are used as commonly as that, so the fact that they are slightly slower shouldn't be too troublesome.) Yea, the speed of the new functions really shouldn't matter. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Andres Freund wrote:
On 2015-01-16 12:56:18 -0300, Alvaro Herrera wrote:
So how about something like
#define ALLOCFLAG_HUGE 0x01
#define ALLOCFLAG_NO_ERROR_ON_OOM 0x02
void *
MemoryContextAllocFlags(MemoryContext context, Size size, int flags);
I don't know, this seems a bit awkward to use. Your earlier example with
the *Huge variant that returns a smaller allocation doesn't really
convince me - that'd need a separate API anyway.
What example was that? My thinking was that the mcxt.c function would
return NULL if the request was not satisfied; only the caller would be
entitled to retry with a smaller size. I was thinking in something like
baseflag = ALLOCFLAG_NO_ERROR_ON_OOM;
reqsz = SomeHugeValue;
while (true)
{
ptr = MemoryContextAllocFlags(cxt, reqsz,
ALLOCFLAG_HUGE | baseflag);
if (ptr != NULL)
break; /* success */
/* too large, retry with a smaller allocation */
reqsz *= 0.75;
/* if under some limit, have it fail next time */
if (reqsz SomeHugeValue * 0.1)
baseflag = 0;
}
/* by here, you know ptr points to a memory area of size reqsz, which is
between SomeHugeValue * 0.1 and SomeHugeValue. */
Were you thinking of something else?
I definitely do not want to push the nofail stuff via the
MemoryContextData- API into aset.c. Imo aset.c should always return
NULL and then mcxt.c should throw the error if in the normal palloc()
function.
Sure, that seems reasonable ...
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Thu, Jan 15, 2015 at 8:42 AM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-15 08:40:34 -0500, Robert Haas wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). palloc_or_null()? That'd work for me, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Robert Haas wrote: Hmm, I understood Tom to be opposing the idea of a palloc variant that returns NULL on failure, and I understand you to be supporting it. But maybe I'm confused. Your understanding seems correct to me. I was just saying that your description of Tom's argument to dislike the idea seemed at odds with what he was actually saying. Anyway, I support it. I agree that there are systems (or circumstances?) where malloc is going to succeed and then the world will blow up later on anyway, but I don't think that means that an out-of-memory error is the only sensible response to a palloc failure; returning NULL seems like a sometimes-useful alternative. I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Fri, Jan 16, 2015 at 12:57 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). I liked palloc_noerror() better myself FWIW. Voting for palloc_noerror() as well. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Wed, Jan 14, 2015 at 9:42 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas wrote: On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: However, there is a larger practical problem with this whole concept, which is that experience should teach us to be very wary of the assumption that asking for memory the system can't give us will just lead to nice neat malloc-returns-NULL behavior. Any small perusal of the mailing list archives will remind you that very often the end result will be SIGSEGV, OOM kills, unrecoverable trap-on-write when the kernel realizes it can't honor a copy-on-write promise, yadda yadda. Agreed that it's arguable that these only occur in misconfigured systems ... but misconfiguration appears to be the default in a depressingly large fraction of systems. (This is another reason for _safe not being the mot juste :-() I don't really buy this. It's pretty incredible to think that after a malloc() failure there is absolutely no hope of carrying on sanely. If that were true, we wouldn't be able to ereport() out-of-memory errors at any severity less than FATAL, but of course it doesn't work that way. Moreover, AllocSetAlloc() contains malloc() and, if that fails, calls malloc() again with a smaller value, without even throwing an error. I understood Tom's point differently: instead of malloc() failing, malloc() will return a supposedly usable pointer, but later usage of it will lead to a crash of some sort. We know this does happen in reality, because people do report it; but we also know how to fix it. And for systems that have been correctly set up, the new behavior (using some plan B for when malloc actually fails instead of spuriously succeeding only to cause a later crash) will be much more convenient. Hmm, I understood Tom to be opposing the idea of a palloc variant that returns NULL on failure, and I understand you to be supporting it. But maybe I'm confused. Anyway, I support it. I agree that there are systems (or circumstances?) where malloc is going to succeed and then the world will blow up later on anyway, but I don't think that means that an out-of-memory error is the only sensible response to a palloc failure; returning NULL seems like a sometimes-useful alternative. I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On 2015-01-15 08:40:34 -0500, Robert Haas wrote: I do think that safe is the wrong suffix. Maybe palloc_soft_fail() or palloc_null() or palloc_no_oom() or palloc_unsafe(). palloc_or_null()? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: However, there is a larger practical problem with this whole concept, which is that experience should teach us to be very wary of the assumption that asking for memory the system can't give us will just lead to nice neat malloc-returns-NULL behavior. Any small perusal of the mailing list archives will remind you that very often the end result will be SIGSEGV, OOM kills, unrecoverable trap-on-write when the kernel realizes it can't honor a copy-on-write promise, yadda yadda. Agreed that it's arguable that these only occur in misconfigured systems ... but misconfiguration appears to be the default in a depressingly large fraction of systems. (This is another reason for _safe not being the mot juste :-() I don't really buy this. It's pretty incredible to think that after a malloc() failure there is absolutely no hope of carrying on sanely. If that were true, we wouldn't be able to ereport() out-of-memory errors at any severity less than FATAL, but of course it doesn't work that way. Moreover, AllocSetAlloc() contains malloc() and, if that fails, calls malloc() again with a smaller value, without even throwing an error. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Robert Haas wrote: On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane t...@sss.pgh.pa.us wrote: However, there is a larger practical problem with this whole concept, which is that experience should teach us to be very wary of the assumption that asking for memory the system can't give us will just lead to nice neat malloc-returns-NULL behavior. Any small perusal of the mailing list archives will remind you that very often the end result will be SIGSEGV, OOM kills, unrecoverable trap-on-write when the kernel realizes it can't honor a copy-on-write promise, yadda yadda. Agreed that it's arguable that these only occur in misconfigured systems ... but misconfiguration appears to be the default in a depressingly large fraction of systems. (This is another reason for _safe not being the mot juste :-() I don't really buy this. It's pretty incredible to think that after a malloc() failure there is absolutely no hope of carrying on sanely. If that were true, we wouldn't be able to ereport() out-of-memory errors at any severity less than FATAL, but of course it doesn't work that way. Moreover, AllocSetAlloc() contains malloc() and, if that fails, calls malloc() again with a smaller value, without even throwing an error. I understood Tom's point differently: instead of malloc() failing, malloc() will return a supposedly usable pointer, but later usage of it will lead to a crash of some sort. We know this does happen in reality, because people do report it; but we also know how to fix it. And for systems that have been correctly set up, the new behavior (using some plan B for when malloc actually fails instead of spuriously succeeding only to cause a later crash) will be much more convenient. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
I wrote: Michael Paquier michael.paqu...@gmail.com writes: Attached is a patch adding the following set of functions for frontend and backends returning NULL instead of reporting ERROR when allocation fails: - palloc_safe - palloc0_safe - repalloc_safe Unimpressed with this naming convention. _unsafe would be nearer the mark ;-) Less snarkily: _noerror would probably fit better with existing precedents in our code. However, there is a larger practical problem with this whole concept, which is that experience should teach us to be very wary of the assumption that asking for memory the system can't give us will just lead to nice neat malloc-returns-NULL behavior. Any small perusal of the mailing list archives will remind you that very often the end result will be SIGSEGV, OOM kills, unrecoverable trap-on-write when the kernel realizes it can't honor a copy-on-write promise, yadda yadda. Agreed that it's arguable that these only occur in misconfigured systems ... but misconfiguration appears to be the default in a depressingly large fraction of systems. (This is another reason for _safe not being the mot juste :-() In that light, I'm not really convinced that there's a safe use-case for a behavior like this. I certainly wouldn't risk asking for a couple of gigabytes on the theory that I could just ask for less if it fails. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Tom Lane writes: [blah] (This is another reason for _safe not being the mot juste :-() My wording was definitely incorrect but I sure you got it: I should have said safe on error. noerror or error_safe would are definitely more correct. In that light, I'm not really convinced that there's a safe use-case for a behavior like this. I certainly wouldn't risk asking for a couple of gigabytes on the theory that I could just ask for less if it fails. That's as well a matter of documentation. We could add a couple of lines in for example xfunc.sgml to describe the limitations of such APIs. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Safe memory allocation functions
Michael Paquier michael.paqu...@gmail.com writes: Attached is a patch adding the following set of functions for frontend and backends returning NULL instead of reporting ERROR when allocation fails: - palloc_safe - palloc0_safe - repalloc_safe Unimpressed with this naming convention. _unsafe would be nearer the mark ;-) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Safe memory allocation functions
Hi all,
For the last couple of weeks it has been mentioned a couple of times
that it would be useful to have a set of palloc APIs able to return
NULL on OOM to allow certain code paths to not ERROR and to take
another route when memory is under pressure. This has been for example
mentioned on the FPW compression thread or here:
http://www.postgresql.org/message-id/cab7npqrbewhsbj_tkaogtpcmrxyjsvkkb9p030d0tpijb4t...@mail.gmail.com
Attached is a patch adding the following set of functions for frontend
and backends returning NULL instead of reporting ERROR when allocation
fails:
- palloc_safe
- palloc0_safe
- repalloc_safe
This has simply needed some refactoring in aset.c to set up the new
functions by passing an additional control flag, and I didn't think
that adding a new safe version for AllocSetContextCreate was worth it.
Those APIs are not called anywhere yet, but I could for example write
a small extension for that that could be put in src/test/modules or
publish on github in my plugin repo. Also, I am not sure if this is
material for 9.5, even if the patch is not complicated, but let me
know if you are interested in it and I'll add it to the next CF.
Regards,
--
Michael
From 008f6bf5a1691fbdf59004157ed521d3b8c41eaf Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Tue, 13 Jan 2015 15:40:38 +0900
Subject: [PATCH] Add safe memory allocation APIs able to return NULL on OOM
The following functions are added to the existing set for frontend and
backend:
- palloc_safe
- palloc0_safe
- repalloc_safe
---
src/backend/utils/mmgr/aset.c| 529 +++
src/backend/utils/mmgr/mcxt.c| 124 +
src/common/fe_memutils.c | 72 --
src/include/common/fe_memutils.h | 3 +
src/include/nodes/memnodes.h | 2 +
src/include/utils/palloc.h | 3 +
6 files changed, 451 insertions(+), 282 deletions(-)
diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
index 85b3c9a..5911c53 100644
--- a/src/backend/utils/mmgr/aset.c
+++ b/src/backend/utils/mmgr/aset.c
@@ -243,11 +243,22 @@ typedef struct AllocChunkData
((AllocPointer)(((char *)(chk)) + ALLOC_CHUNKHDRSZ))
/*
+ * Wrappers for allocation functions.
+ */
+static void *set_alloc_internal(MemoryContext context,
+Size size, bool is_safe);
+static void *set_realloc_internal(MemoryContext context, void *pointer,
+ Size size, bool is_safe);
+
+/*
* These functions implement the MemoryContext API for AllocSet contexts.
*/
static void *AllocSetAlloc(MemoryContext context, Size size);
+static void *AllocSetAllocSafe(MemoryContext context, Size size);
static void AllocSetFree(MemoryContext context, void *pointer);
static void *AllocSetRealloc(MemoryContext context, void *pointer, Size size);
+static void *AllocSetReallocSafe(MemoryContext context,
+ void *pointer, Size size);
static void AllocSetInit(MemoryContext context);
static void AllocSetReset(MemoryContext context);
static void AllocSetDelete(MemoryContext context);
@@ -264,8 +275,10 @@ static void AllocSetCheck(MemoryContext context);
*/
static MemoryContextMethods AllocSetMethods = {
AllocSetAlloc,
+ AllocSetAllocSafe,
AllocSetFree,
AllocSetRealloc,
+ AllocSetReallocSafe,
AllocSetInit,
AllocSetReset,
AllocSetDelete,
@@ -517,140 +530,16 @@ AllocSetContextCreate(MemoryContext parent,
}
/*
- * AllocSetInit
- * Context-type-specific initialization routine.
- *
- * This is called by MemoryContextCreate() after setting up the
- * generic MemoryContext fields and before linking the new context
- * into the context tree. We must do whatever is needed to make the
- * new context minimally valid for deletion. We must *not* risk
- * failure --- thus, for example, allocating more memory is not cool.
- * (AllocSetContextCreate can allocate memory when it gets control
- * back, however.)
- */
-static void
-AllocSetInit(MemoryContext context)
-{
- /*
- * Since MemoryContextCreate already zeroed the context node, we don't
- * have to do anything here: it's already OK.
- */
-}
-
-/*
- * AllocSetReset
- * Frees all memory which is allocated in the given set.
- *
- * Actually, this routine has some discretion about what to do.
- * It should mark all allocated chunks freed, but it need not necessarily
- * give back all the resources the set owns. Our actual implementation is
- * that we hang onto any keeper block specified for the set. In this way,
- * we don't thrash malloc() when a context is repeatedly reset after small
- * allocations, which is typical behavior for per-tuple contexts.
- */
-static void
-AllocSetReset(MemoryContext context)
-{
- AllocSet set = (AllocSet) context;
- AllocBlock block;
-
- AssertArg(AllocSetIsValid(set));
-
-#ifdef MEMORY_CONTEXT_CHECKING
- /* Check for corruption and leaks before freeing */
- AllocSetCheck(context);
-#endif
-
- /* Clear chunk freelists */
- MemSetAligned(set-freelist, 0, sizeof(set-freelist));
-
- block =
Re: [HACKERS] Safe memory allocation functions
Michael Paquier wrote Attached is a patch adding the following set of functions for frontend and backends returning NULL instead of reporting ERROR when allocation fails: - palloc_safe - palloc0_safe - repalloc_safe The only thing I can contribute is paint...I'm not fond of the word _safe and think _try would be more informative...in the spirit of try/catch as a means of error handling/recovery. David J. -- View this message in context: http://postgresql.nabble.com/Safe-memory-allocation-functions-tp5833709p5833711.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
