On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas <robertmh...@gmail.com> wrote:
> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao <masao.fu...@gmail.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
From b9ff4e9ffdc9b02d1ee372c63ee36bfe7659ee9f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Thu, 12 Feb 2015 03:36:55 +0900
Subject: [PATCH 1/2] Add palloc_extended for frontend and backend
This interface can be used to control at a lower level memory allocation
using an interface similar to MemoryContextAllocExtended, but this time
applied to CurrentMemoryContext on backend side, while on frontend side
a similar interface is available.
---
src/backend/utils/mmgr/mcxt.c | 37 +++++++++++++++++++++++++++++++++++++
src/common/fe_memutils.c | 36 ++++++++++++++++++++++++++----------
src/include/common/fe_memutils.h | 11 ++++++++++-
src/include/utils/palloc.h | 1 +
4 files changed, 74 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 202bc78..2589f6a 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -811,6 +811,43 @@ palloc0(Size size)
return ret;
}
+void *
+palloc_extended(Size size, int flags)
+{
+ /* duplicates MemoryContextAllocExtended to avoid increased overhead */
+ void *ret;
+
+ AssertArg(MemoryContextIsValid(CurrentMemoryContext));
+ AssertNotInCriticalSection(CurrentMemoryContext);
+
+ if (((flags & MCXT_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) ||
+ ((flags & MCXT_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size)))
+ elog(ERROR, "invalid memory alloc request size %zu", size);
+
+ CurrentMemoryContext->isReset = false;
+
+ ret = (*CurrentMemoryContext->methods->alloc) (CurrentMemoryContext, size);
+ if (ret == NULL)
+ {
+ if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+ {
+ MemoryContextStats(TopMemoryContext);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed on request of size %zu.", size)));
+ }
+ return NULL;
+ }
+
+ VALGRIND_MEMPOOL_ALLOC(CurrentMemoryContext, ret, size);
+
+ if ((flags & MCXT_ALLOC_ZERO) != 0)
+ MemSetAligned(ret, 0, size);
+
+ return ret;
+}
+
/*
* pfree
* Release an allocated chunk.
diff --git a/src/common/fe_memutils.c b/src/common/fe_memutils.c
index 345221e..bd60cc2 100644
--- a/src/common/fe_memutils.c
+++ b/src/common/fe_memutils.c
@@ -19,8 +19,8 @@
#include "postgres_fe.h"
-void *
-pg_malloc(size_t size)
+static inline void *
+pg_malloc_extended(size_t size, int flags)
{
void *tmp;
@@ -28,22 +28,32 @@ pg_malloc(size_t size)
if (size == 0)
size = 1;
tmp = malloc(size);
- if (!tmp)
+
+ if (tmp == NULL)
{
- fprintf(stderr, _("out of memory\n"));
- exit(EXIT_FAILURE);
+ if ((flags & MCXT_ALLOC_NO_OOM) == 0)
+ {
+ fprintf(stderr, _("out of memory\n"));
+ exit(EXIT_FAILURE);
+ }
+ return NULL;
}
+
+ if ((flags & MCXT_ALLOC_ZERO) != 0)
+ MemSet(tmp, 0, size);
return tmp;
}
void *
-pg_malloc0(size_t size)
+pg_malloc(size_t size)
{
- void *tmp;
+ return pg_malloc_extended(size, 0);
+}
- tmp = pg_malloc(size);
- MemSet(tmp, 0, size);
- return tmp;
+void *
+pg_malloc0(size_t size)
+{
+ return pg_malloc_extended(size, MCXT_ALLOC_ZERO);
}
void *
@@ -109,6 +119,12 @@ palloc0(Size size)
return pg_malloc0(size);
}
+void *
+palloc_extended(Size size, int flags)
+{
+ return pg_malloc_extended(size, flags);
+}
+
void
pfree(void *pointer)
{
diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index f7114c2..7afe3c2 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -16,10 +16,19 @@ extern void *pg_malloc0(size_t size);
extern void *pg_realloc(void *pointer, size_t size);
extern void pg_free(void *pointer);
-/* Equivalent functions, deliberately named the same as backend functions */
+/*
+ * Equivalent functions and flags, deliberately named the same as backend
+ * functions.
+ */
+#define MCXT_ALLOC_HUGE 0x01 /* allow huge allocation (> 1 GB)
+ * not actually used for frontends */
+#define MCXT_ALLOC_NO_OOM 0x02 /* no failure if out-of-memory */
+#define MCXT_ALLOC_ZERO 0x04 /* zero allocated memory */
+
extern char *pstrdup(const char *in);
extern void *palloc(Size size);
extern void *palloc0(Size size);
+extern void *palloc_extended(Size size, int flags);
extern void *repalloc(void *pointer, Size size);
extern void pfree(void *pointer);
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index f586fd5..95b6ef3 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -60,6 +60,7 @@ extern void *MemoryContextAllocExtended(MemoryContext context,
extern void *palloc(Size size);
extern void *palloc0(Size size);
+extern void *palloc_extended(Size size, int flags);
extern void *repalloc(void *pointer, Size size);
extern void pfree(void *pointer);
--
2.3.0
From 386e146584903d01e8cdcb78ecd1b33d37debc01 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Thu, 12 Feb 2015 03:48:18 +0900
Subject: [PATCH 2/2] Rework handling of OOM when allocating record buffer in
XLOG reader
Commit 2c03216 has replaced the memory allocation of the read buffer by a
palloc while it was a malloc previously. However palloc fails unconditionally
if an out-of-memory error shows up instead of returning NULL as a malloc
would do. The broken logic is fixed using palloc_extended which is available
for both frontend and backends, a routine able to not fail should an OOM occur
when doing an allocation.
---
src/backend/access/transam/xlogreader.c | 9 ++++++++-
src/backend/replication/logical/logical.c | 5 +++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 60470b5..4efa198 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -149,7 +149,14 @@ allocate_recordbuf(XLogReaderState *state, uint32 reclength)
if (state->readRecordBuf)
pfree(state->readRecordBuf);
- state->readRecordBuf = (char *) palloc(newSize);
+
+ state->readRecordBuf = (char *) palloc_extended(newSize,
+ MCXT_ALLOC_NO_OOM);
+ if (state->readRecordBuf == NULL)
+ {
+ state->readRecordBufSize = 0;
+ return false;
+ }
state->readRecordBufSize = newSize;
return true;
}
diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 30baa45..774ebbc 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -163,6 +163,11 @@ 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")));
+
ctx->reader->private_data = ctx;
ctx->reorder = ReorderBufferAllocate();
--
2.3.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
