On Mon, Dec 29, 2014 at 8:14 PM, Heikki Linnakangas
<hlinnakan...@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
From 5f22e4d1b202a5234e28fde97fd0a13a6fcf9171 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Mon, 5 Jan 2015 14:15:08 +0900
Subject: [PATCH] Complain about OOM of XLOG reader allocation in logical
decoding code
This will prevent a crash if allocation cannot be done properly by
XLogReaderAllocate as it uses a malloc.
---
src/backend/replication/logical/logical.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 80b6102..ea363c0 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -162,6 +162,12 @@ StartupDecodingContext(List *output_plugin_options,
ctx->slot = slot;
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.")));
+
ctx->reader->private_data = ctx;
ctx->reorder = ReorderBufferAllocate();
--
2.2.1
From 0a208287a9cf1ffa5585ad835c15b5eae4645e8a Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Mon, 5 Jan 2015 14:02:00 +0900
Subject: [PATCH] Fix XLOG reader allocation assuming that palloc can be NULL
2c03216 has updated allocate_recordbuf to use palloc instead of malloc
when allocating a record buffer, falsing the assumption taken by some
code paths expecting an OOM and reporting a dedicated error message
if a NULL pointer was created at allocation, something that cannot
happen with palloc because it always fails in case of an OOM. Hence
remove those checks, and use at the same time repalloc_huge, now needed
as well on frontend side by at least pg_xlogdump to cover as well the
fact that palloc cannot allocate more than 1GB.
---
contrib/pg_xlogdump/pg_xlogdump.c | 2 --
src/backend/access/transam/xlog.c | 8 +-------
src/backend/access/transam/xlogreader.c | 33 ++++++++++-----------------------
src/common/fe_memutils.c | 6 ++++++
src/include/common/fe_memutils.h | 1 +
5 files changed, 18 insertions(+), 32 deletions(-)
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 9f05e25..762269e 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -916,8 +916,6 @@ main(int argc, char **argv)
/* we have everything we need, start reading */
xlogreader_state = XLogReaderAllocate(XLogDumpReadPage, &private);
- if (!xlogreader_state)
- fatal_error("out of memory");
/* first find a valid recptr to start from */
first_record = XLogFindNextRecord(xlogreader_state, private.startptr);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e5dddd4..7d2ca49 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1063,8 +1063,7 @@ XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn)
if (!debug_reader)
debug_reader = XLogReaderAllocate(NULL, NULL);
- if (!debug_reader ||
- !DecodeXLogRecord(debug_reader, (XLogRecord *) recordBuf.data,
+ if (!DecodeXLogRecord(debug_reader, (XLogRecord *) recordBuf.data,
&errormsg))
{
appendStringInfo(&buf, "error decoding record: %s",
@@ -5785,11 +5784,6 @@ StartupXLOG(void)
/* Set up XLOG reader facility */
MemSet(&private, 0, sizeof(XLogPageReadPrivate));
xlogreader = XLogReaderAllocate(&XLogPageRead, &private);
- if (!xlogreader)
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory"),
- errdetail("Failed while allocating an XLog reading processor.")));
xlogreader->system_identifier = ControlFile->system_identifier;
if (read_backup_label(&checkPointLoc, &backupEndRequired,
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 67d6223..71701ff 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -21,7 +21,7 @@
#include "access/xlogreader.h"
#include "catalog/pg_control.h"
-static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
+static void allocate_recordbuf(XLogReaderState *state, uint32 reclength);
static bool ValidXLogPageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogPageHeader hdr);
@@ -94,13 +94,7 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
* Allocate an initial readRecordBuf of minimal size, which can later be
* enlarged if necessary.
*/
- if (!allocate_recordbuf(state, 0))
- {
- pfree(state->errormsg_buf);
- pfree(state->readBuf);
- pfree(state);
- return NULL;
- }
+ allocate_recordbuf(state, 0);
return state;
}
@@ -130,7 +124,6 @@ XLogReaderFree(XLogReaderState *state)
/*
* Allocate readRecordBuf to fit a record of at least the given length.
- * Returns true if successful, false if out of memory.
*
* readRecordBufSize is set to the new buffer size.
*
@@ -139,7 +132,7 @@ XLogReaderFree(XLogReaderState *state)
* with. (That is enough for all "normal" records, but very large commit or
* abort records might need more space.)
*/
-static bool
+static void
allocate_recordbuf(XLogReaderState *state, uint32 reclength)
{
uint32 newSize = reclength;
@@ -147,11 +140,12 @@ allocate_recordbuf(XLogReaderState *state, uint32 reclength)
newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
newSize = Max(newSize, 5 * Max(BLCKSZ, XLOG_BLCKSZ));
- if (state->readRecordBuf)
- pfree(state->readRecordBuf);
- state->readRecordBuf = (char *) palloc(newSize);
+ if (state->readRecordBuf == NULL)
+ state->readRecordBuf = (char *) palloc(newSize);
+ else
+ state->readRecordBuf =
+ (char *) repalloc_huge(state->readRecordBuf, newSize);
state->readRecordBufSize = newSize;
- return true;
}
/*
@@ -302,15 +296,8 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
/*
* Enlarge readRecordBuf as needed.
*/
- if (total_len > state->readRecordBufSize &&
- !allocate_recordbuf(state, total_len))
- {
- /* We treat this as a "bogus data" condition */
- report_invalid_record(state, "record length %u at %X/%X too long",
- total_len,
- (uint32) (RecPtr >> 32), (uint32) RecPtr);
- goto err;
- }
+ if (total_len > state->readRecordBufSize)
+ allocate_recordbuf(state, total_len);
len = XLOG_BLCKSZ - RecPtr % XLOG_BLCKSZ;
if (total_len > len)
diff --git a/src/common/fe_memutils.c b/src/common/fe_memutils.c
index f90ae64..beffe48 100644
--- a/src/common/fe_memutils.c
+++ b/src/common/fe_memutils.c
@@ -126,3 +126,9 @@ repalloc(void *pointer, Size size)
{
return pg_realloc(pointer, size);
}
+
+void *
+repalloc_huge(void *pointer, Size size)
+{
+ return pg_realloc(pointer, size);
+}
diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index 61c1b6f..610a854 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -21,6 +21,7 @@ extern char *pstrdup(const char *in);
extern void *palloc(Size size);
extern void *palloc0(Size size);
extern void *repalloc(void *pointer, Size size);
+extern void *repalloc_huge(void *pointer, Size size);
extern void pfree(void *pointer);
/* sprintf into a palloc'd buffer --- these are in psprintf.c */
--
2.2.1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
