Re: Memory leak from ExecutorState context?

On 3/27/23 23:13, Jehan-Guillaume de Rorthais wrote:
> Hi,
>
> On Mon, 20 Mar 2023 15:12:34 +0100
> Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com> wrote:
>
>> On Mon, 20 Mar 2023 09:32:17 +0100
>> Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>>
>>>>> * Patch 1 could be rebased/applied/backpatched
>>>>
>>>> Would it help if I rebase Patch 1 ("move BufFile stuff into separate
>>>> context")?
>>>
>>> Yeah, I think this is something we'd want to do. It doesn't change the
>>> behavior, but it makes it easier to track the memory consumption etc.
>>
>> Will do this week.
>
> 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 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?

FWIW The comment in hashjoin.h needs updating to reflect the change.

>
> 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.

But I think it's just ugly and overly verbose - it'd be much nicer to
e.g. pass the memory context as a parameter, and do the switch inside.

>
> 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.

> 4. I left the debug messages for some more review rounds
>

OK

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company