Message-ID: <f89c7c65-28f4-82a3-867c-a73e7ab3ccf0@enterprisedb.com>
Date: Sat, 13 May 2023 23:47:53 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: Memory leak from ExecutorState context?
Content-Language: en-US
To: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>,
 Melanie Plageman <melanieplageman@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org,
 Konstantin Knizhnik <knizhnik@garret.ru>,
 Thomas Munro <thomas.munro@gmail.com>
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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <20230510142419.6185a492@karst>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/f89c7c65-28f4-82a3-867c-a73e7ab3ccf0%40enterprisedb.com>
Precedence: bulk

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