Date: Fri, 10 Mar 2023 19:51:14 +0100
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: pgsql-hackers@lists.postgresql.org, Tomas Vondra
 <tomas.vondra@enterprisedb.com>, Melanie Plageman
 <melanieplageman@gmail.com>
Subject: Re: Memory leak from ExecutorState context?
Message-ID: <20230310195114.6d0c5406@karst>
In-Reply-To: <dbae24d7-0dda-18aa-5e08-8138ac1caef9@enterprisedb.com>
References: <20230228190643.1e368315@karst>
	<45d453c8-b2d3-b477-36eb-32fdf4455f3c@enterprisedb.com>
	<20230301184840.0a897a80@karst>
	<3013398b-316c-638f-2a73-3783e8e2ef02@enterprisedb.com>
	<20230302001827.66e95dc3@karst>
	<41c5766d-ed71-b70c-bbbc-d3396c462d62@enterprisedb.com>
	<20230302130838.717e888d@karst>
	<77a96d42-00cb-2448-465a-aa1e92d00cac@enterprisedb.com>
	<20230302191530.781909fe@karst>
	<dbae24d7-0dda-18aa-5e08-8138ac1caef9@enterprisedb.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/20230310195114.6d0c5406%40karst>
Precedence: bulk

Hi,

> So I guess the best thing would be to go through these threads, see what
> the status is, restart the discussion and propose what to do. If you do
> that, I'm happy to rebase the patches, and maybe see if I could improve
> them in some way.

OK! It took me some time, but I did it. I'll try to sum up the situation as
simply as possible.

I reviewed the following threads:

* Out of Memory errors are frustrating as heck!
  2019-04-14 -> 2019-04-28
  https://www.postgresql.org/message-id/flat/bc138e9f-c89e-9147-5395-61d51a=
757b3b%40gusw.net

  This discussion stalled, waiting for OP, but ideas there ignited all other
  discussions.

* accounting for memory used for BufFile during hash joins
  2019-05-04 -> 2019-09-10
  https://www.postgresql.org/message-id/flat/20190504003414.bulcbnge3rhwhcs=
h%40development

  This was suppose to push forward a patch discussed on previous thread, but
  it actually took over it and more ideas pops from there.

* Replace hashtable growEnable flag
  2019-05-15 -> 2019-05-16
  https://www.postgresql.org/message-id/flat/CAB0yrekv%3D6_T_eUe2kOEvWUMwuf=
cvfd15SFmCABtYFOkxCFdfA%40mail.gmail.com

  This one quickly merged to the next one.

* Avoiding hash join batch explosions with extreme skew and weird stats
  2019-05-16 -> 2020-09-24
  https://www.postgresql.org/message-id/flat/CA%2BhUKGKWWmf%3DWELLG%3DaUGbc=
ugRaSQbtm0tKYiBut-B2rVKX63g%40mail.gmail.com

  Another thread discussing another facet of the problem, but eventually en=
d up
  discussing / reviewing the BNLJ implementation.
 =20
Five possible fixes/ideas were discussed all over these threads:


1. "move BufFile stuff into separate context"
   last found patch: 2019-04-21
   https://www.postgresql.org/message-id/20190421114618.z3mpgmimc3rmubi4%40=
development
   https://www.postgresql.org/message-id/attachment/100822/0001-move-BufFil=
e-stuff-into-separate-context.patch

   This patch helps with observability/debug by allocating the bufFiles in =
the
   appropriate context instead of the "ExecutorState" one.

   I suppose this simple one has been forgotten in the fog of all other
   discussions. Also, this probably worth to be backpatched.

2. "account for size of BatchFile structure in hashJoin"
   last found patch: 2019-04-22
   https://www.postgresql.org/message-id/20190428141901.5dsbge2ka3rxmpk6%40=
development
   https://www.postgresql.org/message-id/attachment/100951/v2-simple-rebala=
nce.patch

   This patch seems like a good first step:

   * it definitely helps older versions where other patches discussed are w=
ay
     too invasive to be backpatched
   * it doesn't step on the way of other discussed patches

   While looking at the discussions around this patch, I was wondering if t=
he
   planner considers the memory allocation of bufFiles. But of course, Mela=
nie
   already noticed that long before I was aware of this problem and discuss=
ion:

   2019-07-10: =C2=ABI do think that accounting for Buffile overhead when e=
stimating
   the size of the hashtable during ExecChooseHashTableSize() so it can be
     used during planning is a worthwhile patch by itself (though I know it
     is not even part of this patch).=C2=BB
   https://www.postgresql.org/message-id/CAAKRu_Yiam-%3D06L%2BR8FR%2BVaceb-=
ozQzzMqRiY2pDYku1VdZ%3DEw%40mail.gmail.com
=20
   Tomas Vondra agreed with this in his answer, but no new version of the p=
atch
   where produced.

   Finally, Melanie was pushing the idea to commit this patch no matter oth=
er
   pending patches/ideas:

   2019-09-05: =C2=ABIf Tomas or someone else has time to pick up and modif=
y BufFile
     accounting patch, committing that still seems like the nest logical
     step.=C2=BB
   https://www.postgresql.org/message-id/CAAKRu_b6%2BjC93WP%2BpWxqK5KAZJC5R=
mxm8uquKtEf-KQ%2B%2B1Li6Q%40mail.gmail.com

   Unless I'm wrong, no one down voted this.

3. "per slice overflow file"
   last found patch: 2019-05-08
   https://www.postgresql.org/message-id/20190508150844.rij36rtuk4lhvztw%40=
development
   https://www.postgresql.org/message-id/attachment/101080/v4-per-slice-ove=
rflow-file-20190508.patch

   This patch has been withdraw after an off-list discussion with Thomas Mu=
nro
   because of a missing parallel hashJoin implementation. Plus, before any
   effort started on the parallel implementation, the BNLJ idea appeared and
   seemed more appealing.

   See:
   https://www.postgresql.org/message-id/20190529145517.sj2poqmb3cr4cg6w%40=
development

   By the time, it still seems to have some interest despite the BNLJ patch:

   2019-07-10: =C2=ABIf slicing is made to work for parallel-aware hashjoin=
 and the
   code is in a committable state (and probably has the threshold I mention=
ed
     above), then I think that this patch should go in.=C2=BB
   https://www.postgresql.org/message-id/CAAKRu_Yiam-%3D06L%2BR8FR%2BVaceb-=
ozQzzMqRiY2pDYku1VdZ%3DEw%40mail.gmail.com

   But this might have been disapproved later by Tomas:

   2019-09-10: =C2=ABI have to admit I kinda lost track [...] My feeling is=
 that we
     should get the BNLJ committed first, and then maybe use some of those
     additional strategies as fallbacks (depending on which issues are still
     unsolved by the BNLJ).=C2=BB
   https://www.postgresql.org/message-id/20190910134751.x64idfqj6qgt37om%40=
development

4. "Block Nested Loop Join"
   last found patch: 2020-08-31
   https://www.postgresql.org/message-id/CAAKRu_aLMRHX6_y%3DK5i5wBMTMQvoPMO=
8DT3eyCziTHjsY11cVA%40mail.gmail.com
   https://www.postgresql.org/message-id/attachment/113608/v11-0001-Impleme=
nt-Adaptive-Hashjoin.patch

   Most of the discussion was consideration about the BNLJ parallel and
   semi-join implementation. Melanie put a lot of work on this. This looks =
like
   the most advanced patch so far and add a fair amount of complexity.

   There were some open TODOs, but Melanie was waiting for some more review=
 and
   feedback on v11 first.

5. Only split the skewed batches
   Discussion: 2019-07-11
   https://www.postgresql.org/message-id/CA%2BTgmoYqpbzC1g%2By0bxDFkpM60Kr2=
fnn0hVvT-RfVWonRY2dMA%40mail.gmail.com
   https://www.postgresql.org/message-id/CAB0yremvswRAT86Afb9MZ_PaLHyY9BT31=
3-adCHbhMJ%3Dx_GEcg%40mail.gmail.com

   Robert Haas pointed out that current implementation and discussion were =
not
   really responding to the skew in a very effective way. He's considering
   splitting batches unevenly. Hubert Zhang stepped in, detailed some more =
and
   volunteer to work on such a patch.

   No one reacted.

   It seems to me this is an interesting track to explore. This looks like a
   good complement of 2. ("account for size of BatchFile structure in hashJ=
oin"
   patch). However, this idea probably couldn't be backpatched.

   Note that it could help with 3. as well by slicing only the remaining sk=
ewed
   values.

   Also, this could have some impact on the "Block Nested Loop Join" patch =
if
   the later is kept to deal with the remaining skewed batches.

> I was hoping we'd solve this by the BNL, but if we didn't get that in 4
> years, maybe we shouldn't stall and get at least an imperfect stop-gap
> solution ...

Indeed. So, to sum-up:

* Patch 1 could be rebased/applied/backpatched
* Patch 2 is worth considering to backpatch
* Patch 3 seemed withdrawn in favor of BNLJ
* Patch 4 is waiting for some more review and has some TODO
* discussion 5 worth few minutes to discuss before jumping on previous topi=
cs

1 & 2 are imperfect solution but doesn't weight much and could be backpatch=
ed.
4 & 5 are long-term solutions for a futur major version needing some more
discussions, test and reviews.
3 is not 100% buried, but a last round in the arena might settle its destin=
y for
good.

Hopefully this sum-up is exhaustive and will help clarify this 3-years-old
topic.

Regards,