From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Konstantin Knizhnik <knizhnik@garret.ru>
Cc: Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Memory leak from ExecutorState context?
Date: Sat, 13 May 2023 23:47:53 +0200
Message-ID: <f89c7c65-28f4-82a3-867c-a73e7ab3ccf0@enterprisedb.com> (raw)
In-Reply-To: <20230510142419.6185a492@karst>
References: <455abe0e-91b2-f428-6f4c-b95c7c8dfb52@enterprisedb.com>
<20230320151234.38b2235e@karst>
<20230327231323.08277083@karst>
<f548a8d6-9ac2-46a9-1197-585ad79c1797@enterprisedb.com>
<20230328151745.0f6061f8@karst>
<f532d33d-e4c1-ed00-e047-94f3aafa49db@enterprisedb.com>
<20230331140611.3695d30d@karst>
<20230408020119.32a0841b@karst>
<CAAKRu_ZxT9K9FuUCKV6SwZZnC9ExAiELa1oUYyiPk4eGHh12GQ@mail.gmail.com>
<20230504193006.1b5b9622@karst>
<20230508155648.lzddwywo6emote2b@liskov>
<20230510142419.6185a492@karst>
Hi,
Thanks for the patches. A couple mostly minor comments, to complement
Melanie's review:
0001
I'm not really sure about calling this "hybrid hash-join". What does it
even mean? The new comments simply describe the existing batching, and
how we increment number of batches, etc.
When someone says "hybrid" it usually means a combination of two very
different approaches. Say, a join switching between NL and HJ would be
hybrid. But this is not it.
I'm not against describing the behavior, but the comment either needs to
explain why it's hybrid or it should not be called that.
0002
I wouldn't call the new ExecHashJoinSaveTuple parameter spillcxt but
just something generic (e.g. cxt). Yes, we're passing spillCxt, but from
the function's POV it's just a pointer.
I also wouldn't move the ExecHashJoinSaveTuple comment inside - it just
needs to be reworded that we're expecting the context to be with the
right lifespan. The function comment is the right place to document such
expectations, people are less likely to read the function body.
The modified comment in hashjoin.h has a some alignment issues.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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: tomas.vondra@enterprisedb.com, jgdr@dalibo.com, melanieplageman@gmail.com, knizhnik@garret.ru, thomas.munro@gmail.com
Subject: Re: Memory leak from ExecutorState context?
In-Reply-To: <f89c7c65-28f4-82a3-867c-a73e7ab3ccf0@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox