agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: pgsql-hackers@lists.postgresql.org, Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Subject: Re: Memory leak from ExecutorState context?
Date: Fri, 10 Mar 2023 19:51:14 +0100
Message-ID: <20230310195114.6d0c5406@karst> (raw)
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>
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-61d51a757b3b%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.bulcbnge3rhwhcsh%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_eUe2kOEvWUMwufcvfd15SFmCABtYFOkxCFdfA%40m...
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%3DaUGbcugRaSQbtm0tKYiBut-B2rVKX63g...
Another thread discussing another facet of the problem, but eventually end up
discussing / reviewing the BNLJ implementation.
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%40development
https://www.postgresql.org/message-id/attachment/100822/0001-move-BufFile-stuff-into-separate-contex...
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%40development
https://www.postgresql.org/message-id/attachment/100951/v2-simple-rebalance.patch
This patch seems like a good first step:
* it definitely helps older versions where other patches discussed are way
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 the
planner considers the memory allocation of bufFiles. But of course, Melanie
already noticed that long before I was aware of this problem and discussion:
2019-07-10: «I do think that accounting for Buffile overhead when estimating
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).»
https://www.postgresql.org/message-id/CAAKRu_Yiam-%3D06L%2BR8FR%2BVaceb-ozQzzMqRiY2pDYku1VdZ%3DEw%40...
Tomas Vondra agreed with this in his answer, but no new version of the patch
where produced.
Finally, Melanie was pushing the idea to commit this patch no matter other
pending patches/ideas:
2019-09-05: «If Tomas or someone else has time to pick up and modify BufFile
accounting patch, committing that still seems like the nest logical
step.»
https://www.postgresql.org/message-id/CAAKRu_b6%2BjC93WP%2BpWxqK5KAZJC5Rmxm8uquKtEf-KQ%2B%2B1Li6Q%40...
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%40development
https://www.postgresql.org/message-id/attachment/101080/v4-per-slice-overflow-file-20190508.patch
This patch has been withdraw after an off-list discussion with Thomas Munro
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%40development
By the time, it still seems to have some interest despite the BNLJ patch:
2019-07-10: «If slicing is made to work for parallel-aware hashjoin and the
code is in a committable state (and probably has the threshold I mentioned
above), then I think that this patch should go in.»
https://www.postgresql.org/message-id/CAAKRu_Yiam-%3D06L%2BR8FR%2BVaceb-ozQzzMqRiY2pDYku1VdZ%3DEw%40...
But this might have been disapproved later by Tomas:
2019-09-10: «I 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).»
https://www.postgresql.org/message-id/20190910134751.x64idfqj6qgt37om%40development
4. "Block Nested Loop Join"
last found patch: 2020-08-31
https://www.postgresql.org/message-id/CAAKRu_aLMRHX6_y%3DK5i5wBMTMQvoPMO8DT3eyCziTHjsY11cVA%40mail.g...
https://www.postgresql.org/message-id/attachment/113608/v11-0001-Implement-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%2By0bxDFkpM60Kr2fnn0hVvT-RfVWonRY2dMA%40mail...
https://www.postgresql.org/message-id/CAB0yremvswRAT86Afb9MZ_PaLHyY9BT313-adCHbhMJ%3Dx_GEcg%40mail.g...
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 hashJoin"
patch). However, this idea probably couldn't be backpatched.
Note that it could help with 3. as well by slicing only the remaining skewed
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 topics
1 & 2 are imperfect solution but doesn't weight much and could be backpatched.
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 destiny for
good.
Hopefully this sum-up is exhaustive and will help clarify this 3-years-old
topic.
Regards,
view thread (60+ messages) latest in thread
Message-ID: <20230310195114.6d0c5406@karst>
Permalink: ../20230310195114.6d0c5406@karst/
Also on: postgresql.org/message-id/20230310195114.6d0c5406@karst
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: jgdr@dalibo.com, tomas.vondra@enterprisedb.com, melanieplageman@gmail.com
Subject: Re: Memory leak from ExecutorState context?
In-Reply-To: <20230310195114.6d0c5406@karst>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox