Re: Memory leak from ExecutorState context?

On Tue, 28 Mar 2023 00:43:34 +0200
Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> 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.
> >
> > 1. I'm not sure why we must allocate the "HashBatchFiles" new context
> > under ExecutorState and not under hashtable->hashCxt?
> >
> > The only references I could find was in hashjoin.h:30:
> >
> > /* [...]
> > * [...] (Exception: data associated with the temp files lives in the
> > * per-query context too, since we always call buffile.c in that
> > context.)
> >
> > And in nodeHashjoin.c:1243:ExecHashJoinSaveTuple() (I reworded this
> > original comment in the patch):
> >
> > /* [...]
> > * 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.
> >
> >
> > But these are not explanation of why BufFile related allocations must be
> > under a per-query context.
> >
>
> Doesn't that simply describe the current (unpatched) behavior where
> BufFile is allocated in the per-query context?

I wasn't sure. The first quote from hashjoin.h seems to describe a stronger
rule about «**always** call buffile.c in per-query context». But maybe it ought
to be «always call buffile.c from one of the sub-query context»? 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.
>
> And I'm pretty sure the patch should do
>
> hashtable->fileCxt = AllocSetContextCreate(hashtable->hashCxt,
> "HashBatchFiles",
> ALLOCSET_DEFAULT_SIZES);
>
> 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 confused
about the previous quoted code comments. That's why I kept your original code
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/changes.
> > 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.
> >
>
> 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 needed
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. Let's
see what other reviewers think.

> > 3. You wrote:
> >
> >>> 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 batch
> >>> files gets too high ...
> >
> > So I added a WARNING when batches memory are exhausting the memory size
> > allowed.
> >
> > + if (hashtable->fileCxt->mem_allocated > hashtable->spaceAllowed)
> > + elog(WARNING, "Growing number of hash batch is exhausting
> > memory");
> >
> > This is repeated on each call of ExecHashIncreaseNumBatches when BufFile
> > 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.
> >
>
> 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:

=# 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 memory
(68747392 > 2097152)
WARNING: increasing number of batches from 16384 to 32768
NOTICE: Growing number of hash batch to 32768 is exhausting allowed memory
(137494656 > 2097152)

QUERY PLAN
--------------------------------------------------------------------------
Hash Join (cost=6542057.16..7834651.23 rows=7 width=74)
(actual time=558502.127..724007.708 rows=7040 loops=1)
Hash Cond: (small.id = large.id)
-> Seq Scan on small (cost=0.00..940094.00 rows=94000000 width=41)
(actual time=0.035..3.666 rows=10000 loops=1)
-> Hash (cost=6542057.07..6542057.07 rows=7 width=41)
(actual time=558184.152..558184.153 rows=700000000 loops=1)
Buckets: 32768 (originally 1024)
Batches: 32768 (originally 1)
Memory Usage: 1921kB
-> Seq Scan on large (cost=0.00..6542057.07 rows=7 width=41)
(actual time=0.324..193750.567 rows=700000000 loops=1)
Planning Time: 1.588 ms
Execution Time: 724011.074 ms (8 rows)

Regards,