Re: Memory leak from ExecutorState context?

On Thu, Mar 23, 2023 at 2:49 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
> > On Mon, Mar 20, 2023 at 10:12 AM Jehan-Guillaume de Rorthais
> > <jgdr(at)dalibo(dot)com> wrote:
> >> BNJL and/or other considerations are for 17 or even after. In the meantime,
> >> Melanie, who authored BNLJ, +1 the balancing patch as it can coexists with other
> >> discussed solutions. No one down vote since then. Melanie, what is your
> >> opinion today on this patch? Did you change your mind as you worked for many
> >> months on BNLJ since then?
> >
> > So, in order to avoid deadlock, my design of adaptive hash join/block
> > nested loop hash join required a new parallelism concept not yet in
> > Postgres at the time -- the idea of a lone worker remaining around to do
> > work when others have left.
> >
> > See: BarrierArriveAndDetachExceptLast()
> > introduced in 7888b09994
> >
> > Thomas Munro had suggested we needed to battle test this concept in a
> > more straightforward feature first, so I implemented parallel full outer
> > hash join and parallel right outer hash join with it.
> >
> > https://commitfest.postgresql.org/42/2903/
> >
> > This has been stalled ready-for-committer for two years. It happened to
> > change timing such that it made an existing rarely hit parallel hash
> > join bug more likely to be hit. Thomas recently committed our fix for
> > this in 8d578b9b2e37a4d (last week). It is my great hope that parallel
> > full outer hash join goes in before the 16 feature freeze.
> >
>
> Good to hear this is moving forward.
>
> > If it does, I think it could make sense to try and find committable
> > smaller pieces of the adaptive hash join work. As it is today, parallel
> > hash join does not respect work_mem, and, in some sense, is a bit broken.
> >
> > I would be happy to work on this feature again, or, if you were
> > interested in picking it up, to provide review and any help I can if for
> > you to work on it.
> >
>
> I'm no expert in parallel hashjoin expert, but I'm willing to take a
> look a rebased patch. I'd however recommend breaking the patch into
> smaller pieces - the last version I see in the thread is ~250kB, which
> is rather daunting, and difficult to review without interruption. (I
> don't remember the details of the patch, so maybe that's not possible
> for some reason.)

Great! I will rebase and take a stab at splitting up the patch into
smaller commits, with a focus on finding pieces that may have standalone
benefits, in the 17 dev cycle.

- Melanie