On Fri, Apr 3, 2015 at 5:57 AM, Alvaro Herrera wrote:
> You added this in the worker loop processing each table:
>
> /*
> * Check for config changes before processing each collected
> table.
> */
> if (got_SIGHUP)
> {
> got_SIGHUP = false;
> ProcessConfigFile(PGC_SIGHUP);
>
> /* shutdown requested in config file? */
> if (!AutoVacuumingActive())
> break;
> }
>
> I think bailing out if autovac is disabled is a good idea; for instance,
> if this is a for-wraparound vacuum, we should keep running. My first
> reaction is that this part of the test ought to be moved down a
> screenful or so, until we've ran the recheck step and we have the
> for-wraparound flag in hand; only bail out if that one is not set. But
> on the other hand, maybe the worker will process some tables that are
> not for-wraparound in between some other tables that are, so that might
> turn out to be a bad idea as well. (Though I'm not 100% that it works
> that way; consider commit f51ead09df for instance.) Maybe the test to
> use for this is something along the lines of "if autovacuum was enabled
> before and is no longer enabled, bail out, otherwise keep running".
> This implies that we need to keep track of whether autovac was enabled
> at the start of the worker run.
I did consider the case of wraparound but came to the conclusion that
bailing out as fast as possible was the idea. But well, I guess that
we could play it safe and not add this check after all. The main
use-case of this change is now to be able to do re-balancing of the
cost parameters. We could still decide later if a worker could bail
out earlier or not, depending on what.
> Another thing, but not directly related to this patch: while looking at
> the doc changes, I noticed that we don't have index entries for the
> reloptions we have; for instance, the word "fillfactor" doesn't appear
> in the bookindex.html page at all. Having these things all crammed in
> the CREATE TABLE page seems somewhat poor. I think we could improve on
> this by having a listing of settable parameters in a separate section,
> and have CREATE TABLE, ALTER TABLE, and the Server Configuration chapter
> link to that; we could also have index entries for these items.
> Of course, the simpler fix is to just add a few <indexterm> tags.
This sounds like a good idea, and this refactoring would meritate a
different patch and a different thread. I guess that it should be a
new section in Server Configuration then, named "Relation Options",
with two different subsections for index options and table options.
> As a note, there is no point to "Assert(foo != NULL)" tests when foo is
> later dereferenced in the same routine: the crash is going to happen
> anyway at the dereference, so it's just a LOC uselessly wasted.
Check. I still think that it is useful in this case though... But removed.
> I think this could use some wordsmithing; I didn't like listing the
> parameters explicitely, and Jaime Casanova suggested not using the terms
> "inherit" and "parent table" in this context. So I came up with this:
> Note that the TOAST table uses the parameter values defined for the main
> table, for each parameter applicable to TOAST tables and for which no
> value is set in the TOAST table itself.
Fine for me.
--
Michael
From f43fc9a5a5e0ffb246782010b8860829b8304593 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
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, int flags);
extern void *pg_realloc(void *pointer, size_t size);
extern void pg_free(void *pointer);
@@ -20,6 +30,7 @@ extern void pg_free(void *pointer);
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 2cf5129..9861f0d 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -76,6 +76,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.5
From d85e506baa258643ac820b6f21ac3c86247b3cc2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Fri, 3 Apr 2015 14:28:21 +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 +++++
src/bin/pg_rewind/parsexlog.c | 6 ++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index ba7dfcc..f3cf6a5 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -146,7 +146,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();
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
