Re: Memory leak from ExecutorState context?

On Wed, May 17, 2023 at 07:10:08PM +0200, Jehan-Guillaume de Rorthais wrote:
> On Tue, 16 May 2023 16:00:52 -0400
> Melanie Plageman <melanieplageman@gmail.com> wrote:
> > > From 309ad354b7a9e4dfa01b2985bd883829f5e0eba0 Mon Sep 17 00:00:00 2001
> > > From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> > > Date: Tue, 16 May 2023 15:42:14 +0200
> > > Subject: [PATCH v8 2/3] Allocate hash batches related BufFile in a dedicated
> > There are some existing indentation issues in these files, but you can
> > leave those or put them in a separate commit.
> 
> Reindented in v9.
> 
> I put existing indentation issues in a separate commit to keep the actual
> patches clean from distractions.

It is a matter of opinion, but I tend to prefer the commit with the fix
for the existing indentation issues to be first in the patch set.

> > > diff --git a/src/backend/executor/nodeHashjoin.c
> > > b/src/backend/executor/nodeHashjoin.c @@ -1313,21 +1314,30 @@
> > > 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.
> > > + * If this is the first write to the batch file, this function first
> > > + * create it.  The associated BufFile buffer is allocated in the given
> > > + * context.  It is important to always give the HashSpillContext
> > > + * context.  First to avoid a shorter-lived context, else the temp file
> > > + * buffers will get messed up.  Second, for a better accounting of the
> > > + * spilling memory consumption.
> > > + *
> > >   */  
> > 
> > Here is my suggested wording fot this block comment:
> > 
> > The batch file is lazily created. If this is the first tuple written to
> > this batch, the batch file is created and its buffer is allocated in the
> > given context. It is important to pass in a memory context which will
> > live for the entirety of the lifespan of the batch.
> 
> Reworded. The context must actually survive the batch itself, not just live
> during the lifespan of the batch.

I've added a small recommended change to this inline.

> > >  void
> > >  ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
> > > -                      BufFile **fileptr)
> > > +                      BufFile **fileptr, MemoryContext
> > > cxt) {  
> 
> Note that I changed this to:
> 
>  ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
>                        BufFile **fileptr, HashJoinTable hashtable) {
> 
> As this function must allocate BufFile buffer in spillCxt, I suppose
> we should force it explicitly in the function code.
> 
> Plus, having hashtable locally could be useful for later patch to eg. fine track
> allocated memory in spaceUsed.

I think if you want to pass the hashtable instead of the memory context,
I think you'll need to explain why you still pass the buffile pointer
(because ExecHashJoinSaveTuple() is called for inner and outer batch
files) instead of getting it from the hashtable's arrays of buffile
pointers.

> From c7b70dec3f4c162ea590b53a407c39dfd7ade873 Mon Sep 17 00:00:00 2001
> From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
> Date: Tue, 16 May 2023 15:42:14 +0200
> Subject: [PATCH v9 2/3] Dedicated memory context for hash join spill buffers

> @@ -1310,22 +1311,38 @@ 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.
>   */
>  void
>  ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
> -                      BufFile **fileptr)
> +                      BufFile **fileptr, HashJoinTable hashtable)
>  {
>      BufFile    *file = *fileptr;
>  
>      if (file == NULL)
>      {
> -        /* First write to this batch file, so open it. */
> +        MemoryContext oldctx;
> +
> +        /*
> +         * The batch file is lazily created. If this is the first tuple
> +         * written to this batch, the batch file is created and its buffer is
> +         * allocated in the spillCxt context, NOT in the batchCxt.
> +         *
> +         * During the building phase, inner batch are created with their temp
> +         * file buffers. These buffers are released later, after the batch is
> +         * loaded back to memory during the outer side scan. That explains why
> +         * it is important to use a memory context which live longer than the
> +         * batch itself or some temp file buffers will get messed up.
> +         *
> +         * Also, we use spillCxt instead of hashCxt for a better accounting of
> +         * the spilling memory consumption.
> +         */

Suggested small edit to the second paragraph:

    During the build phase, buffered files are created for inner batches.
    Each batch's buffered file is closed (and its buffer freed) after the
    batch is loaded into memory during the outer side scan. Therefore, it is
    necessary to allocate the batch file buffer in a memory context which
    outlives the batch itself.

I'd also mention the reason for passing the buffile pointer above the
function. I would basically say:

    The data recorded in the file for each tuple is its hash value,
    then the tuple in MinimalTuple format. fileptr may refer to either an
    inner or outer side batch file.

- Melanie