Date: Tue, 28 Mar 2023 16:56:17 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, Justin Pryzby
 <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org, Thomas Munro
 <thomas.munro@gmail.com>
Subject: Re: Memory leak from ExecutorState context?
Message-ID: <20230328165617.3c2c96cf@karst>
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>
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/20230328165617.3c2c96cf%40karst>
Precedence: bulk

Hi,

Sorry for the late answer, I was reviewing the first patch and it took me s=
ome
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=E2=80=AFPM Jehan-Guillaume de Rorthais
> <jgdr@dalibo.com> wrote:
> > > So I guess the best thing would be to go through these threads, see w=
hat
> > > 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 impro=
ve
> > > them in some way. =20
> >
> > OK! It took me some time, but I did it. I'll try to sum up the situatio=
n as
> > simply as possible. =20
>=20
> Wow, so many memories!
>=20
> 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. =20
>=20
> 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"
> > [...]=20
>=20
> 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=E2=80=AFAM Jehan-Guillaume de Rorthais
> <jgdr@dalibo.com> wrote:
> > BNJL and/or other considerations are for 17 or even after. In the meant=
ime,
> > Melanie, who authored BNLJ, +1 the balancing patch as it can coexists w=
ith
> > 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? =20
>=20
> 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.
>=20
> See: BarrierArriveAndDetachExceptLast()
> introduced in 7888b09994
>=20
> 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.
>=20
> https://commitfest.postgresql.org/42/2903/
>=20
> 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.
>=20
> 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. Bu=
t I'm
interested to help, test and review, as far as I can!

Regards,