Re: The return value of allocate_recordbuf()

On Mon, Dec 29, 2014 at 8:14 PM, Heikki Linnakangas
<hlinnakangas@vmware.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