Re: Memory leak from ExecutorState context?

On Wed, 17 May 2023 13:46:35 -0400
Melanie Plageman <melanieplageman@gmail.com> wrote:

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

OK. moved in v10 patch set.

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

Comment added in v10

> > @@ -1310,22 +1311,38 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
> ...
> 
> 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.

Changed.

> I'd also mention the reason for passing the buffile pointer above the
> function.

Added.

Regards,