agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: pgsql-hackers@lists.postgresql.org, Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Memory leak from ExecutorState context?
Date: Tue, 28 Mar 2023 16:56:17 +0200
Message-ID: <20230328165617.3c2c96cf@karst> (raw)
In-Reply-To: <CAAKRu_aACusR5p805RLDUYR+SaEUAu5iTD1cR2CT08m2ve5jBw@mail.gmail.com>
References: <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>
<20230310195114.6d0c5406@karst>
<20230317091834.22e97642@karst>
<ae017eef-79d5-fcd6-b865-b7e55ac5290b@enterprisedb.com>
<ZBdjJ8l3CNmBZUg0@telsasoft.com>
<455abe0e-91b2-f428-6f4c-b95c7c8dfb52@enterprisedb.com>
<20230320151234.38b2235e@karst>
<CAAKRu_aACusR5p805RLDUYR+SaEUAu5iTD1cR2CT08m2ve5jBw@mail.gmail.com>
Hi,
Sorry for the late answer, I was reviewing the first patch and it took me some
time to study and dig around.
On Thu, 23 Mar 2023 08:07:04 -0400
Melanie Plageman <melanieplageman@gmail.com> wrote:
> On Fri, Mar 10, 2023 at 1:51 PM Jehan-Guillaume de Rorthais
> <jgdr@dalibo.com> wrote:
> > > 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.
>
> Wow, so many memories!
>
> I'm excited that someone looked at this old work (though it is sad that
> a customer faced this issue). And, Jehan, I really appreciate your great
> summarization of all these threads. This will be a useful reference.
Thank you!
> > 1. "move BufFile stuff into separate context"
> > [...]
> > I suppose this simple one has been forgotten in the fog of all other
> > discussions. Also, this probably worth to be backpatched.
>
> I agree with Jehan-Guillaume and Tomas that this seems fine to commit
> alone.
This is a WIP.
> > 2. "account for size of BatchFile structure in hashJoin"
> > [...]
>
> I think I would have to see a modern version of a patch which does this
> to assess if it makes sense. But, I probably still agree with 2019
> Melanie :)
I volunteer to work on this after the memory context patch, unless someone grab
it in the meantime.
> [...]
> On Mon, Mar 20, 2023 at 10:12 AM Jehan-Guillaume de Rorthais
> <jgdr@dalibo.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.
This is really interesting to follow. I kinda feel/remember how this could
be useful for your BNLJ patch. It's good to see things are moving, step by
step.
Thanks for the pointers.
> 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 don't think I would be able to pick up such a large and complex patch. But I'm
interested to help, test and review, as far as I can!
Regards,
view thread (60+ messages) latest in thread
Message-ID: <20230328165617.3c2c96cf@karst>
Permalink: ../20230328165617.3c2c96cf@karst/
Also on: postgresql.org/message-id/20230328165617.3c2c96cf@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, melanieplageman@gmail.com, tomas.vondra@enterprisedb.com, pryzby@telsasoft.com, thomas.munro@gmail.com
Subject: Re: Memory leak from ExecutorState context?
In-Reply-To: <20230328165617.3c2c96cf@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