Re: Out of Memory errors are frustrating as heck!

Tomas Vondra <[email protected]>
Newsgroups gmane.comp.db.postgresql.performance
Message-ID <20190421114618.z3mpgmimc3rmubi4@development> 
On Sun, Apr 21, 2019 at 03:08:22AM -0500, Justin Pryzby wrote:
>On Sun, Apr 21, 2019 at 01:03:50AM -0400, Gunther wrote:
>> On 4/20/2019 21:14, Tomas Vondra wrote:
>> >Maybe. But before wasting any more time on the memory leak investigation,
>> >I suggest you first try the patch moving the BufFile allocations to a
>> >separate context. That'll either confirm or disprove the theory.
>>
>> OK, fair enough. So, first patch 0001-* applied, recompiled and
>>
>> 2019-04-21 04:08:04.364 UTC [11304] LOG:  server process (PID 11313) was terminated by signal 11: Segmentation fault
>...
>> turns out the MemoryContext is NULL:
>>
>> (gdb) p context
>> $1 = (MemoryContext) 0x0
>
>I updated Tomas' patch to unconditionally set the context.
>(Note, oldctx vs oldcxt is fairly subtle but I think deliberate?)
>

I don't follow - is there a typo confusing oldctx vs. oldcxt? I don't
think so, but I might have missed something. (I always struggle with which
spelling is the right one).

I think the bug is actually such simpler - the memory context was created
only in ExecuteIncreaseNumBatches() when starting with nbatch=1. But when
the initial nbatch value was higher (i.e. when starting with 2 or more
batches), it was left NULL. That was OK for testing with the contrived
data set, but it may easily break on other examples.

So here is an updated patch - hopefully this version works. I don't have
time to do much more testing now, though. But it compiles.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
0001-move-BufFile-stuff-into-separate-context.patch (text/plain, 6.8 KB) 
From 3292e67135ef67f4bcf1ec0595fe79497c49d47c Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sun, 21 Apr 2019 13:31:14 +0200
Subject: [PATCH 1/2] move BufFile stuff into separate context

---
 src/backend/executor/nodeHash.c     | 54 +++++++++++++++++++++++++++++
 src/backend/executor/nodeHashjoin.c |  7 ++++
 src/include/executor/hashjoin.h     |  1 +
 3 files changed, 62 insertions(+)

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 6ffaa751f2..f7c92e78e9 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -498,6 +498,7 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 	hashtable->skewTuples = 0;
 	hashtable->innerBatchFile = NULL;
 	hashtable->outerBatchFile = NULL;
+	hashtable->fileCtx = NULL;
 	hashtable->spaceUsed = 0;
 	hashtable->spacePeak = 0;
 	hashtable->spaceAllowed = space_allowed;
@@ -527,6 +528,10 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 												"HashBatchContext",
 												ALLOCSET_DEFAULT_SIZES);
 
+	hashtable->fileCtx = AllocSetContextCreate(CurrentMemoryContext,
+								 "HashBatchFiles",
+								 ALLOCSET_DEFAULT_SIZES);
+
 	/* Allocate data that will live for the life of the hashjoin */
 
 	oldcxt = MemoryContextSwitchTo(hashtable->hashCxt);
@@ -559,10 +564,14 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 
 	if (nbatch > 1 && hashtable->parallel_state == NULL)
 	{
+		MemoryContext oldctx;
+
 		/*
 		 * allocate and initialize the file arrays in hashCxt (not needed for
 		 * parallel case which uses shared tuplestores instead of raw files)
 		 */
+		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 		hashtable->innerBatchFile = (BufFile **)
 			palloc0(nbatch * sizeof(BufFile *));
 		hashtable->outerBatchFile = (BufFile **)
@@ -570,6 +579,8 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 		/* The files will not be opened until needed... */
 		/* ... but make sure we have temp tablespaces established for them */
 		PrepareTempTablespaces();
+
+		MemoryContextSwitchTo(oldctx);
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -900,6 +911,11 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 	nbatch = oldnbatch * 2;
 	Assert(nbatch > 1);
 
+	elog(WARNING, "ExecHashIncreaseNumBatches: increasing number of batches from %d to %d", oldnbatch, nbatch);
+
+	elog(LOG, "ExecHashIncreaseNumBatches ======= context stats start =======");
+	MemoryContextStats(TopMemoryContext);
+
 #ifdef HJDEBUG
 	printf("Hashjoin %p: increasing nbatch to %d because space = %zu\n",
 		   hashtable, nbatch, hashtable->spaceUsed);
@@ -909,6 +925,10 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 
 	if (hashtable->innerBatchFile == NULL)
 	{
+		MemoryContext oldctx;
+
+		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 		/* we had no file arrays before */
 		hashtable->innerBatchFile = (BufFile **)
 			palloc0(nbatch * sizeof(BufFile *));
@@ -916,9 +936,15 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 			palloc0(nbatch * sizeof(BufFile *));
 		/* time to establish the temp tablespaces, too */
 		PrepareTempTablespaces();
+
+		MemoryContextSwitchTo(oldctx);
 	}
 	else
 	{
+		MemoryContext oldctx;
+
+		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 		/* enlarge arrays and zero out added entries */
 		hashtable->innerBatchFile = (BufFile **)
 			repalloc(hashtable->innerBatchFile, nbatch * sizeof(BufFile *));
@@ -928,6 +954,9 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 			   (nbatch - oldnbatch) * sizeof(BufFile *));
 		MemSet(hashtable->outerBatchFile + oldnbatch, 0,
 			   (nbatch - oldnbatch) * sizeof(BufFile *));
+
+		MemoryContextSwitchTo(oldctx);
+
 	}
 
 	MemoryContextSwitchTo(oldcxt);
@@ -999,12 +1028,19 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 			}
 			else
 			{
+				MemoryContext oldctx;
+
 				/* dump it out */
 				Assert(batchno > curbatch);
+
+				oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 				ExecHashJoinSaveTuple(HJTUPLE_MINTUPLE(hashTuple),
 									  hashTuple->hashvalue,
 									  &hashtable->innerBatchFile[batchno]);
 
+				MemoryContextSwitchTo(oldctx);
+
 				hashtable->spaceUsed -= hashTupleSize;
 				nfreed++;
 			}
@@ -1042,6 +1078,9 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 			   hashtable);
 #endif
 	}
+
+	elog(LOG, "ExecHashIncreaseNumBatches ======= context stats end =======");
+        MemoryContextStats(TopMemoryContext);
 }
 
 /*
@@ -1656,13 +1695,20 @@ ExecHashTableInsert(HashJoinTable hashtable,
 	}
 	else
 	{
+		MemoryContext oldctx;
+
 		/*
 		 * put the tuple into a temp file for later batches
 		 */
 		Assert(batchno > hashtable->curbatch);
+
+		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 		ExecHashJoinSaveTuple(tuple,
 							  hashvalue,
 							  &hashtable->innerBatchFile[batchno]);
+
+		MemoryContextSwitchTo(oldctx);
 	}
 }
 
@@ -2488,10 +2534,18 @@ ExecHashRemoveNextSkewBucket(HashJoinTable hashtable)
 		}
 		else
 		{
+			MemoryContext oldctx;
+
 			/* Put the tuple into a temp file for later batches */
 			Assert(batchno > hashtable->curbatch);
+
+			oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 			ExecHashJoinSaveTuple(tuple, hashvalue,
 								  &hashtable->innerBatchFile[batchno]);
+
+			MemoryContextSwitchTo(oldctx);
+
 			pfree(hashTuple);
 			hashtable->spaceUsed -= tupleSize;
 			hashtable->spaceUsedSkew -= tupleSize;
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5922e60eed..6a546344ac 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -389,16 +389,23 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel)
 				if (batchno != hashtable->curbatch &&
 					node->hj_CurSkewBucketNo == INVALID_SKEW_BUCKET_NO)
 				{
+					MemoryContext oldctx;
+
 					/*
 					 * Need to postpone this outer tuple to a later batch.
 					 * Save it in the corresponding outer-batch file.
 					 */
 					Assert(parallel_state == NULL);
 					Assert(batchno > hashtable->curbatch);
+
+					oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
+
 					ExecHashJoinSaveTuple(ExecFetchSlotMinimalTuple(outerTupleSlot),
 										  hashvalue,
 										  &hashtable->outerBatchFile[batchno]);
 
+					MemoryContextSwitchTo(oldctx);
+
 					/* Loop around, staying in HJ_NEED_NEW_OUTER state */
 					continue;
 				}
diff --git a/src/include/executor/hashjoin.h b/src/include/executor/hashjoin.h
index a9f9872a78..ef8e4475e8 100644
--- a/src/include/executor/hashjoin.h
+++ b/src/include/executor/hashjoin.h
@@ -328,6 +328,7 @@ typedef struct HashJoinTableData
 	 */
 	BufFile   **innerBatchFile; /* buffered virtual temp file per batch */
 	BufFile   **outerBatchFile; /* buffered virtual temp file per batch */
+	MemoryContext	fileCtx;
 
 	/*
 	 * Info about the datatype-specific hash functions for the datatypes being
-- 
2.20.1
0002-allocate-BufFile-eagerly.patch (text/plain, 2.4 KB) 
From a651fcfdc689059b466b6a7658422f96dd14fc78 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sun, 21 Apr 2019 13:31:53 +0200
Subject: [PATCH 2/2] allocate BufFile eagerly

---
 src/backend/executor/nodeHash.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index f7c92e78e9..4a1f70771d 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -564,6 +564,7 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 
 	if (nbatch > 1 && hashtable->parallel_state == NULL)
 	{
+		int i;
 		MemoryContext oldctx;
 
 		/*
@@ -580,6 +581,15 @@ ExecHashTableCreate(HashState *state, List *hashOperators, bool keepNulls)
 		/* ... but make sure we have temp tablespaces established for them */
 		PrepareTempTablespaces();
 
+		for (i = 1; i < nbatch; i++)
+		{
+			if (!hashtable->innerBatchFile[i])
+				hashtable->innerBatchFile[i] = BufFileCreateTemp(false);
+
+			if (!hashtable->outerBatchFile[i])
+				hashtable->outerBatchFile[i] = BufFileCreateTemp(false);
+		}
+
 		MemoryContextSwitchTo(oldctx);
 	}
 
@@ -925,6 +935,7 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 
 	if (hashtable->innerBatchFile == NULL)
 	{
+		int i;
 		MemoryContext oldctx;
 
 		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
@@ -937,10 +948,20 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 		/* time to establish the temp tablespaces, too */
 		PrepareTempTablespaces();
 
+		for (i = 1; i < nbatch; i++)
+		{
+			if (!hashtable->innerBatchFile[i])
+				hashtable->innerBatchFile[i] = BufFileCreateTemp(false);
+
+			if (!hashtable->outerBatchFile[i])
+				hashtable->outerBatchFile[i] = BufFileCreateTemp(false);
+		}
+
 		MemoryContextSwitchTo(oldctx);
 	}
 	else
 	{
+		int i;
 		MemoryContext oldctx;
 
 		oldctx = MemoryContextSwitchTo(hashtable->fileCtx);
@@ -955,6 +976,15 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable)
 		MemSet(hashtable->outerBatchFile + oldnbatch, 0,
 			   (nbatch - oldnbatch) * sizeof(BufFile *));
 
+		for (i = 1; i < nbatch; i++)
+		{
+			if (!hashtable->innerBatchFile[i])
+				hashtable->innerBatchFile[i] = BufFileCreateTemp(false);
+
+			if (!hashtable->outerBatchFile[i])
+				hashtable->outerBatchFile[i] = BufFileCreateTemp(false);
+		}
+
 		MemoryContextSwitchTo(oldctx);
 
 	}
-- 
2.20.1
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.