Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ph9DB-00087D-4d for pgsql-hackers@arkaria.postgresql.org; Tue, 28 Mar 2023 13:18:05 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ph9D9-0002sS-Vf for pgsql-hackers@arkaria.postgresql.org; Tue, 28 Mar 2023 13:18:03 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ph9D9-0002q2-HP for pgsql-hackers@lists.postgresql.org; Tue, 28 Mar 2023 13:18:03 +0000 Received: from mail1.dalibo.net ([51.159.93.128] helo=mail.dalibo.com) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ph9D6-0006DC-Cz for pgsql-hackers@lists.postgresql.org; Tue, 28 Mar 2023 13:18:03 +0000 Received: from karst (larco.ioguix.net [78.202.0.6]) by mail.dalibo.com (Postfix) with ESMTPSA id CB0471F784; Tue, 28 Mar 2023 15:17:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=dalibo.com; s=a; t=1680009466; bh=TBx07yb6gaaXr+9iNFoURlWxiEwLDk1ZhVC7kscSUs8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=TQ8x/UWwyntV8WA5NYzXzEDYcWgWTPmGpbPKKFoPRjBmsjthlpiODT5lxo+y5ofnD 3KxxSM55AFUlILZs1kCoCRiDw/uoUToVZbOXZ0Cc3F+/TQDbE+fU/e0IZqdhvTmsDA kmN6fHu6HDX7pndhVIyzGMUniSFH/jnvCwy5AKwg= Date: Tue, 28 Mar 2023 15:17:45 +0200 From: Jehan-Guillaume de Rorthais To: Tomas Vondra Cc: Melanie Plageman , pgsql-hackers@lists.postgresql.org Subject: Re: Memory leak from ExecutorState context? Message-ID: <20230328151745.0f6061f8@karst> In-Reply-To: References: <3013398b-316c-638f-2a73-3783e8e2ef02@enterprisedb.com> <20230302001827.66e95dc3@karst> <41c5766d-ed71-b70c-bbbc-d3396c462d62@enterprisedb.com> <20230302130838.717e888d@karst> <77a96d42-00cb-2448-465a-aa1e92d00cac@enterprisedb.com> <20230302191530.781909fe@karst> <20230310195114.6d0c5406@karst> <20230317091834.22e97642@karst> <455abe0e-91b2-f428-6f4c-b95c7c8dfb52@enterprisedb.com> <20230320151234.38b2235e@karst> <20230327231323.08277083@karst> Organization: Dalibo MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="MP_/cg+MWpd=CPm0_9SBk5uZwGx" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --MP_/cg+MWpd=CPm0_9SBk5uZwGx Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Tue, 28 Mar 2023 00:43:34 +0200 Tomas Vondra wrote: > On 3/27/23 23:13, Jehan-Guillaume de Rorthais wrote: > > Please, find in attachment a patch to allocate bufFiles in a dedicated > > context. I picked up your patch, backpatch'd it, went through it and did > > some minor changes to it. I have some comment/questions thought. > >=20 > > 1. I'm not sure why we must allocate the "HashBatchFiles" new context > > under ExecutorState and not under hashtable->hashCxt? > >=20 > > The only references I could find was in hashjoin.h:30: > >=20 > > /* [...] > > * [...] (Exception: data associated with the temp files lives in the > > * per-query context too, since we always call buffile.c in that > > context.) > >=20 > > And in nodeHashjoin.c:1243:ExecHashJoinSaveTuple() (I reworded this > > original comment in the patch): > >=20 > > /* [...] > > * Note: it is important always to call this in the regular executor > > * context, not in a shorter-lived context; else the temp file buffe= rs > > * will get messed up. > >=20 > >=20 > > But these are not explanation of why BufFile related allocations must= be > > under a per-query context.=20 > > =20 >=20 > Doesn't that simply describe the current (unpatched) behavior where > BufFile is allocated in the per-query context?=20 I wasn't sure. The first quote from hashjoin.h seems to describe a stronger rule about =C2=AB**always** call buffile.c in per-query context=C2=BB. But = maybe it ought to be =C2=ABalways call buffile.c from one of the sub-query context=C2=BB? = I assume the aim is to enforce the tmp files removal on query end/error? > I mean, the current code calls BufFileCreateTemp() without switching the > context, so it's in the ExecutorState. But with the patch it very clearly= is > not. >=20 > And I'm pretty sure the patch should do >=20 > hashtable->fileCxt =3D AllocSetContextCreate(hashtable->hashCxt, > "HashBatchFiles", > ALLOCSET_DEFAULT_SIZES); >=20 > and it'd still work. Or why do you think we *must* allocate it under > ExecutorState? That was actually my very first patch and it indeed worked. But I was confu= sed about the previous quoted code comments. That's why I kept your original co= de and decided to rise the discussion here. Fixed in new patch in attachment. > FWIW The comment in hashjoin.h needs updating to reflect the change. Done in the last patch. Is my rewording accurate? > > 2. Wrapping each call of ExecHashJoinSaveTuple() with a memory context > > switch seems fragile as it could be forgotten in futur code path/change= s. > > So I added an Assert() in the function to make sure the current memory > > context is "HashBatchFiles" as expected. > > Another way to tie this up might be to pass the memory context as > > argument to the function. > > ... Or maybe I'm over precautionary. > > =20 >=20 > I'm not sure I'd call that fragile, we have plenty other code that > expects the memory context to be set correctly. Not sure about the > assert, but we don't have similar asserts anywhere else. I mostly sticked it there to stimulate the discussion around this as I need= ed to scratch that itch. > But I think it's just ugly and overly verbose +1 Your patch was just a demo/debug patch by the time. It needed some cleanup = now :) > it'd be much nicer to e.g. pass the memory context as a parameter, and do > the switch inside. That was a proposition in my previous mail, so I did it in the new patch. L= et's see what other reviewers think. > > 3. You wrote: > > =20 > >>> A separate BufFile memory context helps, although people won't see it > >>> unless they attach a debugger, I think. Better than nothing, but I was > >>> wondering if we could maybe print some warnings when the number of ba= tch > >>> files gets too high ... =20 > >=20 > > So I added a WARNING when batches memory are exhausting the memory si= ze > > allowed. > >=20 > > + if (hashtable->fileCxt->mem_allocated > hashtable->spaceAllowed) > > + elog(WARNING, "Growing number of hash batch is exhausting > > memory"); > >=20 > > This is repeated on each call of ExecHashIncreaseNumBatches when BufF= ile > > overflows the memory budget. I realize now I should probably add the > > memory limit, the number of current batch and their memory consumption. > > The message is probably too cryptic for a user. It could probably be > > reworded, but some doc or additionnal hint around this message might = help. > > =20 >=20 > Hmmm, not sure is WARNING is a good approach, but I don't have a better > idea at the moment. I stepped it down to NOTICE and added some more infos. Here is the output of the last patch with a 1MB work_mem: =3D# explain analyze select * from small join large using (id); WARNING: increasing number of batches from 1 to 2 WARNING: increasing number of batches from 2 to 4 WARNING: increasing number of batches from 4 to 8 WARNING: increasing number of batches from 8 to 16 WARNING: increasing number of batches from 16 to 32 WARNING: increasing number of batches from 32 to 64 WARNING: increasing number of batches from 64 to 128 WARNING: increasing number of batches from 128 to 256 WARNING: increasing number of batches from 256 to 512 NOTICE: Growing number of hash batch to 512 is exhausting allowed memory (2164736 > 2097152) WARNING: increasing number of batches from 512 to 1024 NOTICE: Growing number of hash batch to 1024 is exhausting allowed memory (4329472 > 2097152) WARNING: increasing number of batches from 1024 to 2048 NOTICE: Growing number of hash batch to 2048 is exhausting allowed memory (8626304 > 2097152) WARNING: increasing number of batches from 2048 to 4096 NOTICE: Growing number of hash batch to 4096 is exhausting allowed memory (17252480 > 2097152) WARNING: increasing number of batches from 4096 to 8192 NOTICE: Growing number of hash batch to 8192 is exhausting allowed memory (34504832 > 2097152) WARNING: increasing number of batches from 8192 to 16384 NOTICE: Growing number of hash batch to 16384 is exhausting allowed memo= ry (68747392 > 2097152) WARNING: increasing number of batches from 16384 to 32768 NOTICE: Growing number of hash batch to 32768 is exhausting allowed memo= ry (137494656 > 2097152) QUERY PLAN -------------------------------------------------------------------------- Hash Join (cost=3D6542057.16..7834651.23 rows=3D7 width=3D74) (actual time=3D558502.127..724007.708 rows=3D7040 loops=3D1) Hash Cond: (small.id =3D large.id) -> Seq Scan on small (cost=3D0.00..940094.00 rows=3D94000000 width=3D41) (actual time=3D0.035..3.666 rows=3D10000 loops=3D1) -> Hash (cost=3D6542057.07..6542057.07 rows=3D7 width=3D41) (actual time=3D558184.152..558184.153 rows=3D700000000 loops=3D1= )=20 Buckets: 32768 (originally 1024) Batches: 32768 (originally 1) Memory Usage: 1921kB -> Seq Scan on large (cost=3D0.00..6542057.07 rows=3D7 width=3D41) (actual time=3D0.324..193750.567 rows=3D700000000 = loops=3D1) Planning Time: 1.588 ms Execution Time: 724011.074 ms (8 rows) Regards, --MP_/cg+MWpd=CPm0_9SBk5uZwGx Content-Type: text/x-patch Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=0001-Allocate-hash-batches-related-BufFile-in-a-dedicated.patch From b1f9e315cf40b75929431236e200fbdf5a0068c4 Mon Sep 17 00:00:00 2001 From: Jehan-Guillaume de Rorthais Date: Mon, 27 Mar 2023 15:54:39 +0200 Subject: [PATCH] Allocate hash batches related BufFile in a dedicated context --- src/backend/executor/nodeHash.c | 43 +++++++++++++++++++++++++---- src/backend/executor/nodeHashjoin.c | 18 ++++++++---- src/include/executor/hashjoin.h | 15 ++++++++-- src/include/executor/nodeHashjoin.h | 2 +- 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 748c9b0024..3da83ac22a 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -484,7 +484,7 @@ ExecHashTableCreate(HashState *state, List *hashOperators, List *hashCollations, * * The hashtable control block is just palloc'd from the executor's * per-query memory context. Everything else should be kept inside the - * subsidiary hashCxt or batchCxt. + * subsidiary hashCxt, batchCxt or fileCxt. */ hashtable = palloc_object(HashJoinTableData); hashtable->nbuckets = nbuckets; @@ -538,6 +538,10 @@ ExecHashTableCreate(HashState *state, List *hashOperators, List *hashCollations, "HashBatchContext", ALLOCSET_DEFAULT_SIZES); + hashtable->fileCxt = AllocSetContextCreate(CurrentMemoryContext, + "HashBatchFiles", + ALLOCSET_DEFAULT_SIZES); + /* Allocate data that will live for the life of the hashjoin */ oldcxt = MemoryContextSwitchTo(hashtable->hashCxt); @@ -570,15 +574,21 @@ ExecHashTableCreate(HashState *state, List *hashOperators, List *hashCollations, 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->fileCxt); + hashtable->innerBatchFile = palloc0_array(BufFile *, nbatch); hashtable->outerBatchFile = palloc0_array(BufFile *, nbatch); /* The files will not be opened until needed... */ /* ... but make sure we have temp tablespaces established for them */ PrepareTempTablespaces(); + + MemoryContextSwitchTo(oldctx); } MemoryContextSwitchTo(oldcxt); @@ -929,12 +939,18 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable) nbatch = oldnbatch * 2; Assert(nbatch > 1); + elog(WARNING, "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); #endif - oldcxt = MemoryContextSwitchTo(hashtable->hashCxt); + oldcxt = MemoryContextSwitchTo(hashtable->fileCxt); if (hashtable->innerBatchFile == NULL) { @@ -1022,9 +1038,11 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable) { /* dump it out */ Assert(batchno > curbatch); + ExecHashJoinSaveTuple(HJTUPLE_MINTUPLE(hashTuple), hashTuple->hashvalue, - &hashtable->innerBatchFile[batchno]); + &hashtable->innerBatchFile[batchno], + hashtable->fileCxt); hashtable->spaceUsed -= hashTupleSize; nfreed++; @@ -1042,6 +1060,13 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable) oldchunks = nextchunk; } + if (hashtable->fileCxt->mem_allocated > hashtable->spaceAllowed) + elog(NOTICE, + "Growing number of hash batch to %d is exhausting allowed memory (%ld > %ld)", + nbatch, + hashtable->fileCxt->mem_allocated, + hashtable->spaceAllowed); + #ifdef HJDEBUG printf("Hashjoin %p: freed %ld of %ld tuples, space now %zu\n", hashtable, nfreed, ninmemory, hashtable->spaceUsed); @@ -1063,6 +1088,9 @@ ExecHashIncreaseNumBatches(HashJoinTable hashtable) hashtable); #endif } + + elog(LOG, "ExecHashIncreaseNumBatches ======= context stats end ======="); + MemoryContextStats(TopMemoryContext); } /* @@ -1681,9 +1709,11 @@ ExecHashTableInsert(HashJoinTable hashtable, * put the tuple into a temp file for later batches */ Assert(batchno > hashtable->curbatch); + ExecHashJoinSaveTuple(tuple, hashvalue, - &hashtable->innerBatchFile[batchno]); + &hashtable->innerBatchFile[batchno], + hashtable->fileCxt); } if (shouldFree) @@ -2534,8 +2564,11 @@ ExecHashRemoveNextSkewBucket(HashJoinTable hashtable) { /* Put the tuple into a temp file for later batches */ Assert(batchno > hashtable->curbatch); + ExecHashJoinSaveTuple(tuple, hashvalue, - &hashtable->innerBatchFile[batchno]); + &hashtable->innerBatchFile[batchno], + hashtable->fileCxt); + pfree(hashTuple); hashtable->spaceUsed -= tupleSize; hashtable->spaceUsedSkew -= tupleSize; diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index f189fb4d28..6055abde49 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -432,8 +432,10 @@ ExecHashJoinImpl(PlanState *pstate, bool parallel) */ Assert(parallel_state == NULL); Assert(batchno > hashtable->curbatch); + ExecHashJoinSaveTuple(mintuple, hashvalue, - &hashtable->outerBatchFile[batchno]); + &hashtable->outerBatchFile[batchno], + hashtable->fileCxt); if (shouldFree) heap_free_minimal_tuple(mintuple); @@ -1234,21 +1236,27 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate) * The data recorded in the file for each tuple is its hash value, * then the tuple in MinimalTuple format. * - * Note: it is important always to call this in the regular executor - * context, not in a shorter-lived context; else the temp file buffers - * will get messed up. + * Note: it is important always to call this in the HashBatchFiles context, + * not in a shorter-lived context; else the temp file buffers will get messed + * up. */ void ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue, - BufFile **fileptr) + BufFile **fileptr, MemoryContext filecxt) { BufFile *file = *fileptr; if (file == NULL) { + MemoryContext oldctx; + + oldctx = MemoryContextSwitchTo(filecxt); + /* First write to this batch file, so open it. */ file = BufFileCreateTemp(false); *fileptr = file; + + MemoryContextSwitchTo(oldctx); } BufFileWrite(file, &hashvalue, sizeof(uint32)); diff --git a/src/include/executor/hashjoin.h b/src/include/executor/hashjoin.h index acb7592ca0..d759235d7f 100644 --- a/src/include/executor/hashjoin.h +++ b/src/include/executor/hashjoin.h @@ -25,10 +25,14 @@ * * Each active hashjoin has a HashJoinTable control block, which is * palloc'd in the executor's per-query context. All other storage needed - * for the hashjoin is kept in private memory contexts, two for each hashjoin. + * for the hashjoin is kept in private memory contexts, three for each + * hashjoin: + * - HashTableContext (hashCxt): the control block associated to the hash table + * - HashBatchContext (batchCxt): storages for batches + * - HashBatchFiles (fileCxt): storage for temp files buffers + * * This makes it easy and fast to release the storage when we don't need it - * anymore. (Exception: data associated with the temp files lives in the - * per-query context too, since we always call buffile.c in that context.) + * anymore. * * The hashtable contexts are made children of the per-query context, ensuring * that they will be discarded at end of statement even if the join is @@ -39,6 +43,10 @@ * "hashCxt", while storage that is only wanted for the current batch is * allocated in the "batchCxt". By resetting the batchCxt at the end of * each batch, we free all the per-batch storage reliably and without tedium. + * Note that data associated with the temp files lives in the "fileCxt" context + * which lives during the entire join as temp files might need to survives + * batches. These files are explicitly destroyed by calling BufFileClose() + * when the code is done with them. * * During first scan of inner relation, we get its tuples from executor. * If nbatch > 1 then tuples that don't belong in first batch get saved @@ -348,6 +356,7 @@ typedef struct HashJoinTableData MemoryContext hashCxt; /* context for whole-hash-join storage */ MemoryContext batchCxt; /* context for this-batch-only storage */ + MemoryContext fileCxt; /* context for the BufFile related storage */ /* used for dense allocation of tuples (into linked chunks) */ HashMemoryChunk chunks; /* one list for the whole batch */ diff --git a/src/include/executor/nodeHashjoin.h b/src/include/executor/nodeHashjoin.h index d367070883..a8f9ae1989 100644 --- a/src/include/executor/nodeHashjoin.h +++ b/src/include/executor/nodeHashjoin.h @@ -29,6 +29,6 @@ extern void ExecHashJoinInitializeWorker(HashJoinState *state, ParallelWorkerContext *pwcxt); extern void ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue, - BufFile **fileptr); + BufFile **fileptr, MemoryContext filecxt); #endif /* NODEHASHJOIN_H */ -- 2.39.2 --MP_/cg+MWpd=CPm0_9SBk5uZwGx--