On 12/12/2025 11:48, Michael Paquier wrote:
> On Fri, Dec 12, 2025 at 09:00:08AM +0000, Bertrand Drouvot wrote:
>> On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote:
>>> where CStringGetTextDatum() has made a copy of buf.data and assigned to
>>> value[4], however buf.data is never free-ed.
>>
>> I did not look in details but I think that we should be in a short lived
>> memory context here so we generally prefer to avoid using pfree for those cases.
>
> The only thing that does a memory allocation is the StringInfo, why
> would a memory context be worth the complication here?
>
>> That might be a valid reason though. Do you have an idea of the "leak" size
>> based on the number of tuples?
>
> I presume that this just needs to imply a very large index, as we are
> doing a simple loop with the items stored in a tuplestore
> (CStringGetTextDatum does a palloc() for a value so we do not care
> about the contents of the StringInfo).
The function is executed in a short lived ExprContext anyway. It's reset
per row. So if you do something:
select * from
generate_series(1, 10) g,
gist_page_items(get_raw_page('gist_idx', g), 'gist_idx');
the memory context is reset between each row, that is, between every
index page.
It might be nice to pfree it anyway, even though it doesn't accumulate
across calls. If you have 100 tuples on an index page, it uses 100 kB of
memory for no good reason.
If we're going to bother changing this at all, let's consider reusing
the buffer. So instead of initStringInfo()+pfree on every tuple,
allocate it once and use resetStringInfo().
> The relation_close() inconsistency is a fun find. We tend to be
> careful with the APIs when opening relations and the ones that enforce
> relkind checks, at least on style ground.
Yeah, that we should fix, for the sake of consistency.
- Heikki
