Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-03 Thread Michael Paquier 
On Fri, Apr 3, 2015 at 9:57 PM, Fujii Masao  wrote:
> On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-03 Thread Fujii Masao 
On Fri, Apr 3, 2015 at 8:37 PM, Michael Paquier
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-03 Thread Michael Paquier 
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
From 292012c19805777cf17443eccd9b4ef05c5f42ec Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 3 Apr 2015 20:29:39 +0900
Subject: [PATCH] Fix error handling of XLogReaderAllocate in case of OOM

Similarly to previous fix 9b8d478, commit 2c03216 has switched
XLogReaderAllocate() to use a set of palloc calls instead of malloc,
causing any callers of this function to fail with an error instead of
receiving a NULL pointer in case of out-of-memory error. Fix this by
using palloc_extended with MCXT_ALLOC_NO_OOM that will safely return
NULL in case of an OOM, making this routine compatible with previous
major versions.
---
 src/backend/access/transam/xlogreader.c   | 23 ---
 src/backend/replication/logical/logical.c |  5 +
 src/bin/pg_rewind/parsexlog.c |  6 ++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index ffdc975..9047805 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -66,7 +66,11 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
 {
 	XLogReaderState *state;
 
-	state = (XLogReaderState *) palloc0(sizeof(XLogReaderState));
+	state = (XLogReaderState *)
+		palloc_extended(sizeof(XLogReaderState),
+		MCXT_ALLOC_NO_OOM | MCXT_ALLOC_ZERO);
+	if (!state)
+		return NULL;
 
 	state->max_block_id = -1;
 
@@ -77,14 +81,27 @@ XLogReaderAllocate(XLogPageReadCB pagereadfunc, void *private_data)
 	 * isn't guaranteed to have any particular alignment, whereas palloc()
 	 * will provide MAXALIGN'd storage.
 	 */
-	state->readBuf = (char *) palloc(XLOG_BLCKSZ);
+	state->readBuf = (char *) palloc_extended(XLOG_BLCKSZ,
+			  MCXT_ALLOC_NO_OOM);
+	if (!state->readBuf)
+	{
+		pfree(state);
+		return NULL;
+	}
 
 	state->read_page = pagereadfunc;
 	/* system_identifier initialized to zeroes above */
 	state->private_data = private_data;
 	/* ReadRecPtr and EndRecPtr initialized to zeroes above */
 	/* readSegNo, readOff, readLen, readPageTLI initialized to zeroes above */
-	state->errormsg_buf = palloc(MAX_ERRORMSG_LEN + 1);
+	state->errormsg_buf = palloc_extended(MAX_ERRORMSG_LEN + 1,
+		  MCXT_ALLOC_NO_OOM);
+	if (!state->errormsg_buf)
+	{
+		pfree(state->readBuf);
+		pfree(state);
+		return NULL;
+	}
 	state->errormsg_buf[0] = '\0';
 
 	/*
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();
diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 0787ca1..3cf96ab 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -70,6 +70,8 @@ extractPageMap(const char *datadir, XLogRecPtr startpoint, TimeLineID tli,
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	do
 	{
@@ -121,6 +123,8 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, TimeLineID tli)
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	record = XLogReadRecord(xlogreader, ptr, &errormsg);
 	if (record == NULL)
@@ -171,6 +175,8 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, TimeLineID tli,
 	private.datadir = datadir;
 	private.tli = tli;
 	xlogreader = XLogReaderAllocate(&SimpleXLogPageRead, &private);
+	if (xlogreader == NULL)
+		pg_fatal("out of memory");
 
 	searchptr = forkptr;
 	for (;;)
-- 
2.3.5


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-03 Thread Fujii Masao 
On Fri, Apr 3, 2015 at 2:30 PM, Michael Paquier
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-02 Thread Michael Paquier 
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
From f43fc9a5a5e0ffb246782010b8860829b8304593 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 3 Apr 2015 14:21:12 +0900
Subject: [PATCH 1/2] Add palloc_extended and pg_malloc_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 | 43 ++--
 src/include/common/fe_memutils.h | 11 ++
 src/include/utils/palloc.h   |  1 +
 4 files changed, 81 insertions(+), 11 deletions(-)

diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e2fbfd4..c42a6b6 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -864,6 +864,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..527f9c8 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_internal(size_t size, int flags)
 {
 	void	   *tmp;
 
@@ -28,22 +28,37 @@ 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_malloc(size_t size)
+{
+	return pg_malloc_internal(size, 0);
+}
+
+void *
 pg_malloc0(size_t size)
 {
-	void	   *tmp;
+	return pg_malloc_internal(size, MCXT_ALLOC_ZERO);
+}
 
-	tmp = pg_malloc(size);
-	MemSet(tmp, 0, size);
-	return tmp;
+void *
+pg_malloc_extended(size_t size, int flags)
+{
+	return pg_malloc_internal(size, flags);
 }
 
 void *
@@ -100,13 +115,19 @@ pg_free(void *ptr)
 void *
 palloc(Size size)
 {
-	return pg_malloc(size);
+	return pg_malloc_internal(size, 0);
 }
 
 void *
 palloc0(Size size)
 {
-	return pg_malloc0(size);
+	return pg_malloc_internal(size, MCXT_ALLOC_ZERO);
+}
+
+void *
+palloc_extended(Size size, int flags)
+{
+	return pg_malloc_internal(size, flags);
 }
 
 void
diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index db7cb3e..6466167 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -9,10 +9,20 @@
 #ifndef FE_MEMUTILS_H
 #define FE_MEMUTILS_H
 
+/*
+ * Flags for pg_malloc_extended and palloc_extended, deliberately named
+ * the same as the backend flags.
+ */
+#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 */
+
 /* "Safe" memory allocation functions --- these exit(1) on failure */
 extern char *pg_strdup(const char *in);
 extern void *pg_malloc(size_t size);
 extern void *pg_malloc0(size_t size);
+extern void *pg_malloc_extended(size_t size

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-02 Thread Fujii Masao 
On Thu, Feb 12, 2015 at 4:02 PM, Michael Paquier
 wrote:
> On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas  wrote:
>>
>> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
>>  wrote:
>> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao 
>> > 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-01 Thread Michael Paquier 
On Thu, Apr 2, 2015 at 9:10 AM, Bruce Momjian  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

Re: [HACKERS] The return value of allocate_recordbuf()

2015-04-01 Thread Bruce Momjian 
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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-02-11 Thread Michael Paquier 
On Wed, Feb 11, 2015 at 2:13 AM, Robert Haas  wrote:

> On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
>  wrote:
> > On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao 
> 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 
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

Re: [HACKERS] The return value of allocate_recordbuf()

2015-02-10 Thread Robert Haas 
On Mon, Feb 9, 2015 at 7:02 AM, Michael Paquier
 wrote:
> On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-02-09 Thread Michael Paquier 
On Mon, Feb 9, 2015 at 7:58 PM, Fujii Masao  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-02-09 Thread Fujii Masao 
On Mon, Jan 5, 2015 at 1:25 PM, Michael Paquier
 wrote:
> On Thu, Jan 1, 2015 at 1:10 AM, Robert Haas  wrote:
>> On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
>>  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
*** a/src/backend/access/transam/xlogreader.c
--- b/src/backend/access/transam/xlogreader.c
***
*** 149,155  allocate_recordbuf(XLogReaderState *state, uint32 reclength)
  
  	if (state->readRecordBuf)
  		pfree(state->readRecordBuf);
! 	state->readRecordBuf = (char *) palloc(newSize);
  	state->readRecordBufSize = newSize;
  	return true;
  }
--- 149,163 
  
  	if (state->readRecordBuf)
  		pfree(state->readRecordBuf);
! 	state->readRecordBuf =
! 		(char *) MemoryContextAllocExtended(CurrentMemoryContext,
! 			newSize,
! 			MCXT_ALLOC_NO_OOM);
! 	if (state->readRecordBuf == NULL)
! 	{
! 		state->readRecordBufSize = 0;
! 		return false;
! 	}
  	state->readRecordBufSize = newSize;
  	return true;
  }

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-01-08 Thread Andres Freund 
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2015-01-04 Thread Michael Paquier 
On Mon, Dec 29, 2014 at 8:14 PM, Heikki Linnakangas
 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 
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 
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 e54..7d2ca49 100644
--- a/src/backend/acce

Re: [HACKERS] The return value of allocate_recordbuf()

2015-01-04 Thread Michael Paquier 
On Thu, Jan 1, 2015 at 1:10 AM, Robert Haas  wrote:
> On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2014-12-31 Thread Robert Haas 
On Mon, Dec 29, 2014 at 6:14 AM, Heikki Linnakangas
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] The return value of allocate_recordbuf()

2014-12-29 Thread Heikki Linnakangas 

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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] The return value of allocate_recordbuf()

2014-12-25 Thread Fujii Masao 
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers