On 06/21/2014 01:58 PM, Heikki Linnakangas wrote:
It's a bit difficult to attach the mark to the palloc calls, as neither
the WAL_DEBUG or LWLOCK_STATS code is calling palloc directly, but
marking specific MemoryContexts as sanctioned ought to work. I'll take a
stab at that.
I came up with the attached patch. It adds a function called
MemoryContextAllowInCriticalSection(), which can be used to exempt
specific memory contexts from the assertion. The following contexts are
exempted:
* ErrorContext
* MdCxt, which is used in checkpointer to absorb fsync requests. (the
checkpointer process as a whole is no longer exempt)
* The temporary StringInfos used in WAL_DEBUG (a new memory "WAL Debug"
context is now created for them)
* LWLock stats hash table (a new "LWLock stats" context is created for it)
Barring objections, I'll commit this to master, and remove the assertion
from REL9_4_STABLE.
- Heikki
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index abc5682..e141a28 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -60,6 +60,7 @@
#include "storage/spin.h"
#include "utils/builtins.h"
#include "utils/guc.h"
+#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/relmapper.h"
#include "utils/snapmgr.h"
@@ -1258,6 +1259,25 @@ begin:;
if (XLOG_DEBUG)
{
StringInfoData buf;
+ static MemoryContext walDebugCxt = NULL;
+ MemoryContext oldCxt;
+
+ /*
+ * Allocations within a critical section are normally not allowed,
+ * because allocation failure would lead to a PANIC. But this is just
+ * debugging code that no-one is going to enable in production, so we
+ * don't care. Use a memory context that's exempt from the rule.
+ */
+ if (walDebugCxt == NULL)
+ {
+ walDebugCxt = AllocSetContextCreate(TopMemoryContext,
+ "WAL Debug",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ MemoryContextAllowInCriticalSection(walDebugCxt, true);
+ }
+ oldCxt = MemoryContextSwitchTo(walDebugCxt);
initStringInfo(&buf);
appendStringInfo(&buf, "INSERT @ %X/%X: ",
@@ -1282,10 +1302,11 @@ begin:;
appendStringInfoString(&buf, " - ");
RmgrTable[rechdr->xl_rmid].rm_desc(&buf, (XLogRecord *) recordbuf.data);
- pfree(recordbuf.data);
}
elog(LOG, "%s", buf.data);
- pfree(buf.data);
+
+ MemoryContextSwitchTo(oldCxt);
+ MemoryContextReset(walDebugCxt);
}
#endif
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index d23ac62..debce86 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -106,6 +106,7 @@ typedef struct lwlock_stats
static int counts_for_pid = 0;
static HTAB *lwlock_stats_htab;
+static MemoryContext lwlock_stats_cxt;
#endif
#ifdef LOCK_DEBUG
@@ -143,16 +144,34 @@ init_lwlock_stats(void)
{
HASHCTL ctl;
- if (lwlock_stats_htab != NULL)
+ if (lwlock_stats_cxt != NULL)
{
- hash_destroy(lwlock_stats_htab);
+ MemoryContextDelete(lwlock_stats_cxt);
+ lwlock_stats_cxt = NULL;
+ /* the hash table went away with the context */
lwlock_stats_htab = NULL;
}
+ /*
+ * The LWLock stats will be updated within a critical section, which
+ * requires allocating new hash entries. Allocations within a critical
+ * section are normally not allowed because running out of memory would
+ * lead to a PANIC, but LWLOCK_STATS is debugging code that's not normally
+ * turned on in production, so that's an acceptable risk. The hash entries
+ * are small, so the risk of running out of memory is minimal in practice.
+ */
+ lwlock_stats_cxt = AllocSetContextCreate(TopMemoryContext,
+ "LWLock stats",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ MemoryContextAllowInCriticalSection(lwlock_stats_cxt, true);
+
MemSet(&ctl, 0, sizeof(ctl));
ctl.keysize = sizeof(lwlock_stats_key);
ctl.entrysize = sizeof(lwlock_stats);
ctl.hash = tag_hash;
+ ctl.hcxt = lwlock_stats_cxt;
lwlock_stats_htab = hash_create("lwlock stats", 16384, &ctl,
HASH_ELEM | HASH_FUNCTION);
counts_for_pid = MyProcPid;
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 3c1c81a..4264373 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -219,6 +219,16 @@ mdinit(void)
&hash_ctl,
HASH_ELEM | HASH_FUNCTION | HASH_CONTEXT);
pendingUnlinks = NIL;
+
+ /*
+ * XXX: The checkpointer needs to add entries to the pending ops
+ * table when absorbing fsync requests. That is done within a critical
+ * section. It means that there's a theoretical possibility that you
+ * run out of memory while absorbing fsync requests, which leads to
+ * a PANIC. Fortunately the hash table is small so that's unlikely to
+ * happen in practice.
+ */
+ MemoryContextAllowInCriticalSection(MdCxt, true);
}
}
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index e83e76d..65f072d 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -60,15 +60,9 @@ static void MemoryContextStatsInternal(MemoryContext context, int level);
* You should not do memory allocations within a critical section, because
* an out-of-memory error will be escalated to a PANIC. To enforce that
* rule, the allocation functions Assert that.
- *
- * There are a two exceptions: 1) error recovery uses ErrorContext, which
- * has some memory set aside so that you don't run out. And 2) checkpointer
- * currently just hopes for the best, which is wrong and ought to be fixed,
- * but it's a known issue so let's not complain about in the meanwhile.
*/
#define AssertNotInCriticalSection(context) \
- Assert(CritSectionCount == 0 || (context) == ErrorContext || \
- AmCheckpointerProcess())
+ Assert(CritSectionCount == 0 || (context)->allowInCriticalSection)
/*****************************************************************************
* EXPORTED ROUTINES *
@@ -120,7 +114,10 @@ MemoryContextInit(void)
* require it to contain at least 8K at all times. This is the only case
* where retained memory in a context is *essential* --- we want to be
* sure ErrorContext still has some memory even if we've run out
- * elsewhere!
+ * elsewhere! Also, allow allocations in ErrorContext within a critical
+ * section. Otherwise a PANIC will cause an assertion failure in the
+ * error reporting code, before printing out the real cause of the
+ * failure.
*
* This should be the last step in this function, as elog.c assumes memory
* management works once ErrorContext is non-null.
@@ -130,6 +127,7 @@ MemoryContextInit(void)
8 * 1024,
8 * 1024,
8 * 1024);
+ MemoryContextAllowInCriticalSection(ErrorContext, true);
}
/*
@@ -306,6 +304,26 @@ MemoryContextSetParent(MemoryContext context, MemoryContext new_parent)
}
/*
+ * MemoryContextAllowInCriticalSection
+ * Allow/disallow allocations in this memory context within a critical
+ * section.
+ *
+ * Normally, memory allocations are not allowed within a critical section,
+ * because a failure would lead to PANIC. There are a few exceptions to that,
+ * like allocations related to debugging code that is not supposed to be
+ * enabled in production. This functions can be used to exempt specific
+ * memory contexts from the assertion in palloc().
+ */
+void
+MemoryContextAllowInCriticalSection(MemoryContext context, bool allow)
+{
+ AssertArg(MemoryContextIsValid(context));
+#ifdef USE_ASSERT_CHECKING
+ context->allowInCriticalSection = allow;
+#endif
+}
+
+/*
* GetMemoryChunkSpace
* Given a currently-allocated chunk, determine the total space
* it occupies (including all memory-allocation overhead).
diff --git a/src/include/nodes/memnodes.h b/src/include/nodes/memnodes.h
index f79ebd4..ad77509 100644
--- a/src/include/nodes/memnodes.h
+++ b/src/include/nodes/memnodes.h
@@ -60,6 +60,9 @@ typedef struct MemoryContextData
MemoryContext nextchild; /* next child of same parent */
char *name; /* context name (just for debugging) */
bool isReset; /* T = no space alloced since last reset */
+#ifdef USE_ASSERT_CHECKING
+ bool allowInCritSection; /* allow palloc in critical section */
+#endif
} MemoryContextData;
/* utils/palloc.h contains typedef struct MemoryContextData *MemoryContext */
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 59d0aec..2fede86 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -101,6 +101,8 @@ extern MemoryContext GetMemoryChunkContext(void *pointer);
extern MemoryContext MemoryContextGetParent(MemoryContext context);
extern bool MemoryContextIsEmpty(MemoryContext context);
extern void MemoryContextStats(MemoryContext context);
+extern void MemoryContextAllowInCriticalSection(MemoryContext context,
+ bool allow);
#ifdef MEMORY_CONTEXT_CHECKING
extern void MemoryContextCheck(MemoryContext context);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
