Обсуждение: 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_tkAogtpcMrxYJsvKKB9p030d0TpijB4t3YA@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
Вложения
Michael Paquier <michael.paquier@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
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.
I wrote:
> Michael Paquier <michael.paquier@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
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
On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane <tgl@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
Robert Haas wrote: > On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane <tgl@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jan 14, 2015 at 9:42 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas wrote: >> On Tue, Jan 13, 2015 at 10:10 AM, Tom Lane <tgl@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
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
On Thu, Jan 15, 2015 at 8:42 AM, Andres Freund <andres@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
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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 16, 2015 at 12:57 AM, Alvaro Herrera <alvherre@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
On Fri, Jan 16, 2015 at 8:47 AM, Michael Paquier <michael.paquier@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
Вложения
On 2015-01-16 08:47:10 +0900, Michael Paquier wrote: > On Fri, Jan 16, 2015 at 12:57 AM, Alvaro Herrera > <alvherre@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
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
On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera <alvherre@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
Robert Haas wrote: > On Thu, Jan 15, 2015 at 10:57 AM, Alvaro Herrera > <alvherre@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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 > > <alvherre@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
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 > > > <alvherre@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 Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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_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. 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
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 Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 16, 2015 at 10:56 AM, Alvaro Herrera <alvherre@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
On Sat, Jan 17, 2015 at 11:06 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Jan 16, 2015 at 10:56 AM, Alvaro Herrera > <alvherre@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
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
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
On Tue, Jan 27, 2015 at 5:34 PM, Andres Freund <andres@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
Вложения
On Wed, Jan 28, 2015 at 9:34 AM, Michael Paquier <michael.paquier@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
On Fri, Jan 30, 2015 at 12:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jan 28, 2015 at 9:34 AM, Michael Paquier
> <michael.paquier@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.
Thanks for the clarifications!
>> 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, ...).
Fine for me, any extension or module can still define their own
equivalent of palloc_noerror or whatever using the Extended interface.
>> 3) 0003 adds MemoryContextAllocExtended
> 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.
Yes, this refactoring was good for testing actually... I have changed
the flags as follows, appending MCXT_ seemed cleaner after waking up
this morning:
+#define MCXT_ALLOC_HUGE 0x01 /* huge allocation */
+#define MCXT_ALLOC_NO_OOM 0x02 /* no failure if
out-of-memory */
+#define MCXT_ALLOC_ZERO 0x04 /* clear allocated
memory using
+
* MemSetAligned */
+#define MCXT_ALLOC_ZERO_ALIGNED 0x08 /* clear allocated memory using
+
* MemSetLoop */
--
Michael
Вложения
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
Вложения
On Fri, Jan 30, 2015 at 1:10 AM, Michael Paquier <michael.paquier@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
On Sat, Jan 31, 2015 at 2:58 AM, Robert Haas <robertmhaas@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