Re: The return value of allocate_recordbuf()

Hi,

While reviewing FPW compression patch, I found that allocate_recordbuf()
always returns TRUE though its source code comment says that FALSE is
returned if out of memory. Its return value is checked in two places, but
which is clearly useless.

allocate_recordbuf() was introduced by 7fcbf6a, and then changed by
2c03216 so that palloc() is used instead of malloc and FALSE is never returned
even if out of memory. So this seems an oversight of 2c03216. Maybe
we should change it so that it checks whether we can enlarge the memory
with the requested size before actually allocating the memory?

Regards,

--
Fujii Masao

On 12/26/2014 09:31 AM, Fujii Masao wrote:
> Hi,
>
> While reviewing FPW compression patch, I found that allocate_recordbuf()
> always returns TRUE though its source code comment says that FALSE is
> returned if out of memory. Its return value is checked in two places, but
> which is clearly useless.
>
> allocate_recordbuf() was introduced by 7fcbf6a, and then changed by
> 2c03216 so that palloc() is used instead of malloc and FALSE is never returned
> even if out of memory. So this seems an oversight of 2c03216. Maybe
> we should change it so that it checks whether we can enlarge the memory
> with the requested size before actually allocating the memory?

Hmm. There is no way to check beforehand if a palloc() will fail because
of OOM. We could check for MaxAllocSize, though.

Actually, before 2c03216, when we used malloc() here, the maximum record
size was 4GB. Now it's only 1GB, because of MaxAllocSize. Are we OK with
that, or should we use palloc_huge?

- Heikki

On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Hmm. There is no way to check beforehand if a palloc() will fail because of
> OOM. We could check for MaxAllocSize, though.

I think we need a version of palloc that returns NULL instead of
throwing an error. The error-throwing behavior is for the best in
almost every case, but I think the no-error version would find enough
users to be worthwhile.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 1, 2015 at 1:10 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)com> wrote:
>> Hmm. There is no way to check beforehand if a palloc() will fail because of
>> OOM. We could check for MaxAllocSize, though.
>
> I think we need a version of palloc that returns NULL instead of
> throwing an error. The error-throwing behavior is for the best in
> almost every case, but I think the no-error version would find enough
> users to be worthwhile.
Compression is one of those areas, be it compression of WAL or another
type. The new API would allow to fallback to the non-compression code
path if buffer allocation for compression cannot be done because of an
OOM.

FWIW, I actually looked at how to do that a couple of weeks back, and
you just need a wrapper function, whose content is the existing
AllocSetAlloc, taking an additional boolean flag to trigger an ERROR
or leave with NULL if an OOM appears. On top of that we will need a
new method in MemoryContextMethods, let's call it alloc_safe, for its
equivalent, the new palloc_safe.
--
Michael

On Mon, Dec 29, 2014 at 8:14 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 12/26/2014 09:31 AM, Fujii Masao wrote:
>>
>> Hi,
>>
>> While reviewing FPW compression patch, I found that allocate_recordbuf()
>> always returns TRUE though its source code comment says that FALSE is
>> returned if out of memory. Its return value is checked in two places, but
>> which is clearly useless.
>>
>> allocate_recordbuf() was introduced by 7fcbf6a, and then changed by
>> 2c03216 so that palloc() is used instead of malloc and FALSE is never
>> returned
>> even if out of memory. So this seems an oversight of 2c03216. Maybe
>> we should change it so that it checks whether we can enlarge the memory
>> with the requested size before actually allocating the memory?
>
>
> Hmm. There is no way to check beforehand if a palloc() will fail because of
> OOM. We could check for MaxAllocSize, though.
>
> Actually, before 2c03216, when we used malloc() here, the maximum record
> size was 4GB. Now it's only 1GB, because of MaxAllocSize. Are we OK with
> that, or should we use palloc_huge?

IMO, we should use repalloc_huge, and remove the status checks for
allocate_recordbuf and XLogReaderAllocate, relying on the fact that we
*will* report a failure if we have an OOM instead of returning a
pointer NULL. That's for example something logical.c relies on,
ctx->reader cannot be NULL (adding Andres in CC about that btw):
ctx->reader = XLogReaderAllocate(read_page, ctx);
ctx->reader->private_data = ctx;
Note that the other code paths return an OOM error message if the
reader allocated is NULL.

Speaking of which, attached are two patches.

The first one is for master implementing the idea above, making all
the previous OOM messages being handled by palloc & friends instead of
having each code path reporting the OOM individually with specific
message, and using repalloc_huge to cover the fact that we cannot
allocate more than 1GB with palloc.

Note that for 9.4, I think that we should complain about an OOM in
logical.c where malloc is used as now process would simply crash if
NULL is returned by XLogReaderAllocate. That's the object of the
second patch.

Thoughts?
--
Michael

Hi,

On 2015-01-05 14:18:35 +0900, Michael Paquier wrote:
> Note that for 9.4, I think that we should complain about an OOM in
> logical.c where malloc is used as now process would simply crash if
> NULL is returned by XLogReaderAllocate. That's the object of the
> second patch.

Yes, that's clearly an oversight...

> ctx->reader = XLogReaderAllocate(read_page, ctx);
> + if (!ctx->reader)
> + ereport(ERROR,
> + (errcode(ERRCODE_OUT_OF_MEMORY),
> + errmsg("out of memory"),
> + errdetail("Failed while allocating an XLog reading processor.")));
> +

I've removed the errdetail() as a) its content is quite confusing b) we
don't add error details that don't add more information than the
function name already does as it's implicitly included in the logging.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Jan 5, 2015 at 1:25 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Jan 1, 2015 at 1:10 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)com> wrote:
>>> Hmm. There is no way to check beforehand if a palloc() will fail because of
>>> OOM. We could check for MaxAllocSize, though.
>>
>> I think we need a version of palloc that returns NULL instead of
>> throwing an error. The error-throwing behavior is for the best in
>> almost every case, but I think the no-error version would find enough
>> users to be worthwhile.
> Compression is one of those areas, be it compression of WAL or another
> type. The new API would allow to fallback to the non-compression code
> path if buffer allocation for compression cannot be done because of an
> OOM.
>
> FWIW, I actually looked at how to do that a couple of weeks back, and
> you just need a wrapper function, whose content is the existing
> AllocSetAlloc, taking an additional boolean flag to trigger an ERROR
> or leave with NULL if an OOM appears. On top of that we will need a
> new method in MemoryContextMethods, let's call it alloc_safe, for its
> equivalent, the new palloc_safe.

MemoryContextAllocExtended() was added, so isn't it time to replace palloc()
with MemoryContextAllocExtended(CurrentMemoryContext, MCXT_ALLOC_NO_OOM)
in allocate_recordbuf()?

Regards,

--
Fujii Masao

On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> MemoryContextAllocExtended() was added, so isn't it time to replace palloc()
> with MemoryContextAllocExtended(CurrentMemoryContext, MCXT_ALLOC_NO_OOM)
> in allocate_recordbuf()?

Yeah, let's move on here, but not with this patch I am afraid as this
breaks any client utility using xlogreader.c in frontend, like
pg_xlogdump or pg_rewind because MemoryContextAllocExtended is not
available in frontend, only in backend. We are going to need something
like palloc_noerror, defined on both backend (mcxt.c) and frontend
(fe_memutils.c) side.

Btw, the huge flag should be used as well as palloc only allows
allocation up to 1GB, and this is incompatible with ~9.4 where malloc
is used.
--
Michael

On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> MemoryContextAllocExtended() was added, so isn't it time to replace palloc()
>> with MemoryContextAllocExtended(CurrentMemoryContext, MCXT_ALLOC_NO_OOM)
>> in allocate_recordbuf()?
>
> Yeah, let's move on here, but not with this patch I am afraid as this
> breaks any client utility using xlogreader.c in frontend, like
> pg_xlogdump or pg_rewind because MemoryContextAllocExtended is not
> available in frontend, only in backend. We are going to need something
> like palloc_noerror, defined on both backend (mcxt.c) and frontend
> (fe_memutils.c) side.

Unfortunately, that gets us back into the exact terminological dispute
that we were hoping to avoid. Perhaps we could compromise on
palloc_extended().

> Btw, the huge flag should be used as well as palloc only allows
> allocation up to 1GB, and this is incompatible with ~9.4 where malloc
> is used.

I think that flag should be used only if it's needed, not just on the
grounds that 9.4 has no such limit. In general, limiting allocations
to 1GB has been good to us; for example, it prevents a corrupted TOAST
length from causing a memory allocation large enough to crash the
machine (yes, there are machines you can crash with a giant memory
allocation!). We should bypass that limit only where it is clearly
necessary.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
> wrote:
> >> MemoryContextAllocExtended() was added, so isn't it time to replace
> palloc()
> >> with MemoryContextAllocExtended(CurrentMemoryContext, MCXT_ALLOC_NO_OOM)
> >> in allocate_recordbuf()?
> >
> > Yeah, let's move on here, but not with this patch I am afraid as this
> > breaks any client utility using xlogreader.c in frontend, like
> > pg_xlogdump or pg_rewind because MemoryContextAllocExtended is not
> > available in frontend, only in backend. We are going to need something
> > like palloc_noerror, defined on both backend (mcxt.c) and frontend
> > (fe_memutils.c) side.
>
> Unfortunately, that gets us back into the exact terminological dispute
> that we were hoping to avoid. Perhaps we could compromise on
> palloc_extended().
>

Yes, why not using palloc_extended instead of palloc_noerror that has been
clearly rejected in the other thread. Now, for palloc_extended we should
copy the flags of MemoryContextAllocExtended to fe_memutils.h and have the
same interface between frontend and backend (note that MCXT_ALLOC_HUGE has
no real utility for frontends). Attached is a patch to achieve that,
completed with a second patch for the problem related to this thread. Note
that in the second patch I have added an ERROR in logical.c after calling
XLogReaderAllocate, this was missing..

> Btw, the huge flag should be used as well as palloc only allows
> > allocation up to 1GB, and this is incompatible with ~9.4 where malloc
> > is used.
>
> I think that flag should be used only if it's needed, not just on the
> grounds that 9.4 has no such limit. In general, limiting allocations
> to 1GB has been good to us; for example, it prevents a corrupted TOAST
> length from causing a memory allocation large enough to crash the
> machine (yes, there are machines you can crash with a giant memory
> allocation!). We should bypass that limit only where it is clearly
> necessary.
>

Fine for me to error out in this code path if we try more than 1GB of
allocation.
Regards,
--
Michael

On Thu, Feb 12, 2015 at 04:02:52PM +0900, Michael Paquier wrote:
> Yes, why not using palloc_extended instead of palloc_noerror that has been
> clearly rejected in the other thread. Now, for palloc_extended we should copy
> the flags of MemoryContextAllocExtended to fe_memutils.h and have the same
> interface between frontend and backend (note that MCXT_ALLOC_HUGE has no real
> utility for frontends). Attached is a patch to achieve that, completed with a
> second patch for the problem related to this thread. Note that in the second
> patch I have added an ERROR in logical.c after calling XLogReaderAllocate, this
> was missing..
>
>
> > Btw, the huge flag should be used as well as palloc only allows
> > allocation up to 1GB, and this is incompatible with ~9.4 where malloc
> > is used.
>
> I think that flag should be used only if it's needed, not just on the
> grounds that 9.4 has no such limit. In general, limiting allocations
> to 1GB has been good to us; for example, it prevents a corrupted TOAST
> length from causing a memory allocation large enough to crash the
> machine (yes, there are machines you can crash with a giant memory
> allocation!). We should bypass that limit only where it is clearly
> necessary.
>
>
> Fine for me to error out in this code path if we try more than 1GB of
> allocation.

Where are we on this?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Thu, Apr 2, 2015 at 9:10 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

> Where are we on this?
>

If we want to have allocate_recordbuf error out properly on frontend side,
we are going to need a equivalent of MemoryContextAllocExtended for
frontends in the shape of palloc_extended able to take control flags.
That's what the patch I sent previously proposes. And this is 9.5 material,
except if we accept that allocate_recordbuf() will fail all the time in
case of an OOM (the only code path where we don't need to mandatory fail
with OOM for this routine is used only with WAL_DEBUG in xlog.c). Now if we
push forward with this patch, and I think we should to maintain
compatibility with WAL_DEBUG with previous versions, we'll need to add as
well an ERROR when an OOM occurs after XLogReaderAllocate in logical.c, and
in pg_rewind's parsexlog.c.
--
Michael

On Thu, Feb 12, 2015 at 4:02 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> > wrote:
>> >> MemoryContextAllocExtended() was added, so isn't it time to replace
>> >> palloc()
>> >> with MemoryContextAllocExtended(CurrentMemoryContext,
>> >> MCXT_ALLOC_NO_OOM)
>> >> in allocate_recordbuf()?
>> >
>> > Yeah, let's move on here, but not with this patch I am afraid as this
>> > breaks any client utility using xlogreader.c in frontend, like
>> > pg_xlogdump or pg_rewind because MemoryContextAllocExtended is not
>> > available in frontend, only in backend. We are going to need something
>> > like palloc_noerror, defined on both backend (mcxt.c) and frontend
>> > (fe_memutils.c) side.
>>
>> Unfortunately, that gets us back into the exact terminological dispute
>> that we were hoping to avoid. Perhaps we could compromise on
>> palloc_extended().
>
>
> Yes, why not using palloc_extended instead of palloc_noerror that has been
> clearly rejected in the other thread. Now, for palloc_extended we should
> copy the flags of MemoryContextAllocExtended to fe_memutils.h and have the
> same interface between frontend and backend (note that MCXT_ALLOC_HUGE has
> no real utility for frontends). Attached is a patch to achieve that,
> completed with a second patch for the problem related to this thread. Note
> that in the second patch I have added an ERROR in logical.c after calling
> XLogReaderAllocate, this was missing..

The first patch looks good to me basically. But I have one comment:
shouldn't we expose pg_malloc_extended as a global function like
we did pg_malloc? Some frontends might need to use it in the future.

Regards,

--
Fujii Masao

On Fri, Apr 3, 2015 at 12:56 PM, Fujii Masao wrote:
> The first patch looks good to me basically. But I have one comment:
> shouldn't we expose pg_malloc_extended as a global function like
> we did pg_malloc? Some frontends might need to use it in the future.

Yes, it makes sense as the other functions do it too. So I refactored
the patch and defined a new static inline routine,
pg_malloc_internal(), that all the other functions call to reduce the
temperature in this code path that I suppose can become quite hot even
for frontends. In the second patch I added as well what was needed for
pg_rewind.
--
Michael

On Fri, Apr 3, 2015 at 2:30 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Apr 3, 2015 at 12:56 PM, Fujii Masao wrote:
>> The first patch looks good to me basically. But I have one comment:
>> shouldn't we expose pg_malloc_extended as a global function like
>> we did pg_malloc? Some frontends might need to use it in the future.
>
> Yes, it makes sense as the other functions do it too. So I refactored
> the patch and defined a new static inline routine,
> pg_malloc_internal(), that all the other functions call to reduce the
> temperature in this code path that I suppose can become quite hot even
> for frontends. In the second patch I added as well what was needed for
> pg_rewind.

Thanks for updating the patches!
I pushed the first and a part of the second patch.

Regarding the second patch, you added the checks of the return value of
XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
uses palloc(), but don't we need to replace it with palloc_extended(), too?

Regards,

--
Fujii Masao

On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
> Regarding the second patch, you added the checks of the return value of
> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
> uses palloc(), but don't we need to replace it with palloc_extended(), too?

Doh, you are right. I missed three places. Attached is a new patch
completing the fix.
--
Michael

On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
>> Regarding the second patch, you added the checks of the return value of
>> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
>> uses palloc(), but don't we need to replace it with palloc_extended(), too?
>
> Doh, you are right. I missed three places. Attached is a new patch
> completing the fix.

Thanks for the patch! I updated two source code comments and
changed the log message when XLogReaderAllocate returns NULL
within XLOG_DEBUG block. Just pushed.

Regards,

--
Fujii Masao

On Fri, Apr 3, 2015 at 9:57 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Apr 3, 2015 at 6:35 PM, Fujii Masao wrote:
>>> Regarding the second patch, you added the checks of the return value of
>>> XLogReaderAllocate(). But it seems half-baked. XLogReaderAllocate() still
>>> uses palloc(), but don't we need to replace it with palloc_extended(), too?
>>
>> Doh, you are right. I missed three places. Attached is a new patch
>> completing the fix.
>
> Thanks for the patch! I updated two source code comments and
> changed the log message when XLogReaderAllocate returns NULL
> within XLOG_DEBUG block. Just pushed.

Yes, thanks. This looks good as is.
--
Michael