Fix Error Message for allocate_recordbuf() Failure
Hello,
Attached is a patch for xlogreader.c for a more informative error message
for allocate_recordbuf() failure.
The patch details are:
- Project name.: None
- Uniquely identifiable file name, so we can tell difference between
your v1 and v24.: src/backend/access/transam/xlogreader.c
- What the patch does in a short paragraph.:
The patch enhances the error message when memory cannot be allocated to
extend the readRecordBuf. The previous error message was implying an
invalid (too long) record length and also did not specify how much memory
we were trying to allocate when the memory allocation failure happened.
We have observed failed allocations in production systems where the
reclength was only 344 bytes, but the extended allocation of 5 *
Max(BLKSIZE, XLOG_BLCKSZ) failed due to out of memory.
- Whether the patch is for discussion or for application (see WIP notes
below): for application.
- Which branch the patch is against (ordinarily this will be *master*).
For more on branches in PostgreSQL, see Using Back Branches
<https://wiki.postgresql.org/wiki/Working_with_Git#Using_Back_Branches>.:
master
- Whether it compiles and tests successfully, so we know nothing obvious
is broken.: Yes. We ran make check and all 166 tests passed.
- Whether it contains any platform-specific items and if so, has it been
tested on other platforms.: Nothing platform specific.
- Confirm that the patch includes regression tests
<https://wiki.postgresql.org/wiki/Regression_test_authoring> to check
the new feature actually works as described.: None.
- Include documentation on how to use the new feature, including
examples. See the documentation documentation
<http://www.postgresql.org/docs/current/static/docguide.html> for more
information.: None needed.
- Describe the effect your patch has on performance, if any.: None.
- Try to include a few lines about why you chose to do things particular
ways, rather than let your reviewer guess what was happening. This can be
done as code comments, but it might also be an additional reviewers' guide,
or additions to a README file in one of the code directories.:
Besides making the error message more informative, we had to modify
allocate_recordbuf() to return the actual number of bytes that were being
allocated.
- If your patch addresses a Todo
<https://wiki.postgresql.org/wiki/Todo> item,
please give the name of the Todo item in your email. This is so that the
reviewers will know that the item needs to be marked as done if your patch
is applied.: None.
Thank you.
Shoaib Lari
Attachments:
fix_error_message_for_allocate_recordbuf_failure.patchapplication/octet-stream; name=fix_error_message_for_allocate_recordbuf_failure.patchDownload
commit 4b4e926fe8c805a7b3abcc8def4009177aa3210a
Author: Shoaib Lari <slari@pivotal.io>
Date: Tue Jul 5 10:47:42 2016 -0700
Fix error message for record length too long.
Make the error message more descriptive when cannot enlarge buffer inside
allocate_recordbuf().
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index dcf747c..ae8a82e 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -25,7 +25,8 @@
#include "common/pg_lzcompress.h"
#include "replication/origin.h"
-static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
+static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength,
+ uint32 *newSizeOut);
static bool ValidXLogPageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogPageHeader hdr);
@@ -110,7 +111,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))
+ if (!allocate_recordbuf(state, 0, NULL))
{
pfree(state->errormsg_buf);
pfree(state->readBuf);
@@ -143,7 +144,8 @@ XLogReaderFree(XLogReaderState *state)
/*
* Allocate readRecordBuf to fit a record of at least the given length.
- * Returns true if successful, false if out of memory.
+ * Returns true if successful, false if out of memory. If newSizeOut is
+ * non-NULL, it will be set to the size of the attempted allocation.
*
* readRecordBufSize is set to the new buffer size.
*
@@ -153,13 +155,16 @@ XLogReaderFree(XLogReaderState *state)
* abort records might need more space.)
*/
static bool
-allocate_recordbuf(XLogReaderState *state, uint32 reclength)
+allocate_recordbuf(XLogReaderState *state, uint32 reclength, uint32 *newSizeOut)
{
uint32 newSize = reclength;
newSize += XLOG_BLCKSZ - (newSize % XLOG_BLCKSZ);
newSize = Max(newSize, 5 * Max(BLCKSZ, XLOG_BLCKSZ));
+ if (newSizeOut)
+ *newSizeOut = newSize;
+
if (state->readRecordBuf)
pfree(state->readRecordBuf);
state->readRecordBuf =
@@ -201,6 +206,7 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
uint32 pageHeaderSize;
bool gotheader;
int readOff;
+ uint32 newSizeOut;
/*
* randAccess indicates whether to verify the previous-record pointer of
@@ -334,12 +340,12 @@ XLogReadRecord(XLogReaderState *state, XLogRecPtr RecPtr, char **errormsg)
* Enlarge readRecordBuf as needed.
*/
if (total_len > state->readRecordBufSize &&
- !allocate_recordbuf(state, total_len))
+ !allocate_recordbuf(state, total_len, &newSizeOut))
{
- /* 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);
+ report_invalid_record(state,
+ "cannot allocate %u bytes for record length %u at %X/%X",
+ newSizeOut, total_len, (uint32) (RecPtr >> 32),
+ (uint32) RecPtr);
goto err;
}
On Sat, Jul 9, 2016 at 2:58 AM, Shoaib Lari <slari@pivotal.io> wrote:
Besides making the error message more informative, we had to modify
allocate_recordbuf() to return the actual number of bytes that were being
allocated.
- report_invalid_record(state, "record length %u at %X/%X too long",
- total_len,
- (uint32) (RecPtr >> 32), (uint32) RecPtr);
+ report_invalid_record(state,
+ "cannot allocate %u bytes for record
length %u at %X/%X",
+ newSizeOut, total_len, (uint32) (RecPtr >> 32),
+ (uint32) RecPtr);It does not look like a good idea to me to complicate the interface of
allocate_recordbuf just to make more verbose one error message,
meaning that it basically a complain related to the fact that
palloc_extended(MCXT_ALLOC_NO_OOM) does not mention to the user the
size that it has tried to allocate before returning NULL. Do you have
a use case that would prove to be useful if this extra piece of
information is provided? Because it seems to me that we don't really
care if we know how much memory it has failed to allocate, we only
want to know that it *has* failed and take actions only based on that.
And even if we make this thing more verbose, a better approach would
be surely to generate a WARNING message for backends in mcxt.c and
have something printed to stderr for frontends in fe_memutils.c
without calling exit().
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11 July 2016 at 12:04, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sat, Jul 9, 2016 at 2:58 AM, Shoaib Lari <slari@pivotal.io> wrote:
Besides making the error message more informative, we had to modify
allocate_recordbuf() to return the actual number of bytes that were being
allocated.- report_invalid_record(state, "record length %u at %X/%X too long", - total_len, - (uint32) (RecPtr >> 32), (uint32) RecPtr); + report_invalid_record(state, + "cannot allocate %u bytes for record length %u at %X/%X", + newSizeOut, total_len, (uint32) (RecPtr >> 32), + (uint32) RecPtr);It does not look like a good idea to me to complicate the interface of
allocate_recordbuf just to make more verbose one error message,
meaning that it basically a complain related to the fact that
palloc_extended(MCXT_ALLOC_NO_OOM) does not mention to the user the
size that it has tried to allocate before returning NULL. Do you have
a use case that would prove to be useful if this extra piece of
information is provided? Because it seems to me that we don't really
care if we know how much memory it has failed to allocate, we only
want to know that it *has* failed and take actions only based on that.
I actually find details of how much memory we tried to allocate to be
really useful in other places that do emit it. Sometimes it's been an
important clue when trying to figure out what's going on on a remote system
with no or limited access. Even if it just tells me "we were probably
incrementally allocating lots of small values since this failure is small"
vs "this allocation is huge, look for something unusually large" or "this
allocation is impossibly huge / some suspicious value look for memory
corruption".
I tend to agree with your suggestion about a better approach but do think
emitting details on allocation failures is useful. At least when people
turn VM overcommit off ...
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Thank you for the feedback.
We had encountered this error message when allocating a record buf of 344
bytes. The message "record length 344 at %X/%X too long" along with the
comment /* We treat this as a "bogus data" condition */ masked the OOM
condition, implying an error in log record size calculation logic.
The actual allocation that failed was for 5 * Max(BLCKSZ, XLOG_BLCKSZ), a
much larger allocation.
Shoaib
On Sun, Jul 10, 2016 at 10:58 PM, Craig Ringer <craig@2ndquadrant.com>
wrote:
Show quoted text
On 11 July 2016 at 12:04, Michael Paquier <michael.paquier@gmail.com>
wrote:On Sat, Jul 9, 2016 at 2:58 AM, Shoaib Lari <slari@pivotal.io> wrote:
Besides making the error message more informative, we had to modify
allocate_recordbuf() to return the actual number of bytes that werebeing
allocated.
- report_invalid_record(state, "record length %u at %X/%X too long", - total_len, - (uint32) (RecPtr >> 32), (uint32) RecPtr); + report_invalid_record(state, + "cannot allocate %u bytes for record length %u at %X/%X", + newSizeOut, total_len, (uint32) (RecPtr >> 32), + (uint32) RecPtr);It does not look like a good idea to me to complicate the interface of
allocate_recordbuf just to make more verbose one error message,
meaning that it basically a complain related to the fact that
palloc_extended(MCXT_ALLOC_NO_OOM) does not mention to the user the
size that it has tried to allocate before returning NULL. Do you have
a use case that would prove to be useful if this extra piece of
information is provided? Because it seems to me that we don't really
care if we know how much memory it has failed to allocate, we only
want to know that it *has* failed and take actions only based on that.I actually find details of how much memory we tried to allocate to be
really useful in other places that do emit it. Sometimes it's been an
important clue when trying to figure out what's going on on a remote system
with no or limited access. Even if it just tells me "we were probably
incrementally allocating lots of small values since this failure is small"
vs "this allocation is huge, look for something unusually large" or "this
allocation is impossibly huge / some suspicious value look for memory
corruption".I tend to agree with your suggestion about a better approach but do think
emitting details on allocation failures is useful. At least when people
turn VM overcommit off ...--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Jul 11, 2016 at 12:04 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Sat, Jul 9, 2016 at 2:58 AM, Shoaib Lari <slari@pivotal.io> wrote:
Besides making the error message more informative, we had to modify
allocate_recordbuf() to return the actual number of bytes that were being
allocated.- report_invalid_record(state, "record length %u at %X/%X too long", - total_len, - (uint32) (RecPtr >> 32), (uint32) RecPtr); + report_invalid_record(state, + "cannot allocate %u bytes for record length %u at %X/%X", + newSizeOut, total_len, (uint32) (RecPtr >> 32), + (uint32) RecPtr);It does not look like a good idea to me to complicate the interface of
allocate_recordbuf just to make more verbose one error message, ...
For what it's worth, I think it's fine. Good error messages are a useful thing.
More generally, I think the whole patch looks good and should be committed.
--
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
On Wed, Sep 21, 2016 at 12:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
For what it's worth, I think it's fine. Good error messages are a useful thing.
More generally, I think the whole patch looks good and should be committed.
Hm. I'd think that it is still more portable to just issue a WARNING
message in palloc_extended() when MCXT_ALLOC_NO_OOM then. Other code
paths could benefit from that as well, and the patch proposed does
nothing for the other places calling it. I am fine to write a patch
for this purpose if you agree on that.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 21, 2016 at 10:25 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Sep 21, 2016 at 12:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
For what it's worth, I think it's fine. Good error messages are a useful thing.
More generally, I think the whole patch looks good and should be committed.
Hm. I'd think that it is still more portable to just issue a WARNING
message in palloc_extended() when MCXT_ALLOC_NO_OOM then. Other code
paths could benefit from that as well, and the patch proposed does
nothing for the other places calling it. I am fine to write a patch
for this purpose if you agree on that.
Or in short the attached. All the other callers of palloc_extended
benefit from that.
--
Michael
Attachments:
palloc-oom-warning.patchtext/plain; charset=US-ASCII; name=palloc-oom-warning.patchDownload
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 5cf388f..52b9c1b 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -973,6 +973,10 @@ palloc_extended(Size size, int flags)
errmsg("out of memory"),
errdetail("Failed on request of size %zu.", size)));
}
+ ereport(WARNING,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed on request of size %zu.", size)));
return NULL;
}
diff --git a/src/common/fe_memutils.c b/src/common/fe_memutils.c
index 58c5c4c..0d44ef6 100644
--- a/src/common/fe_memutils.c
+++ b/src/common/fe_memutils.c
@@ -30,9 +30,9 @@ pg_malloc_internal(size_t size, int flags)
tmp = malloc(size);
if (tmp == NULL)
{
+ fprintf(stderr, _("could not allocate %zu bytes of memory\n"), size);
if ((flags & MCXT_ALLOC_NO_OOM) == 0)
{
- fprintf(stderr, _("out of memory\n"));
exit(EXIT_FAILURE);
}
return NULL;
On Tue, Sep 20, 2016 at 9:25 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Wed, Sep 21, 2016 at 12:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
For what it's worth, I think it's fine. Good error messages are a useful thing.
More generally, I think the whole patch looks good and should be committed.
Hm. I'd think that it is still more portable to just issue a WARNING
message in palloc_extended() when MCXT_ALLOC_NO_OOM then. Other code
paths could benefit from that as well, and the patch proposed does
nothing for the other places calling it. I am fine to write a patch
for this purpose if you agree on that.
No, I strongly disagree with that. I think when you pass
MCXT_ALLOC_NO_OOM, you're saying that you are prepared to deal with a
NULL return value. It becomes your job to decide whether to emit any
log message and which one to emit. If palloc_extended() insists on
emitting a warning regardless, the caller can't now emit a more
specific message without creating redundant log chatter.
--
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