Обсуждение: Fix memory leak in gist_page_items() of pageinspect
Hi Hackers,
While reading the code of pageinspect, I just found a memory leak in gist_page_items():
```
values[4] = CStringGetTextDatum(buf.data);
nulls[4] = false;
nulls[4] = false;
```
where CStringGetTextDatum() has made a copy of buf.data and assigned to value[4], however buf.data is never free-ed. This leak is inside a per-tuple loop, thus it should be fixed.
In the meantime, the other small issue was also found in the same function. An index is opened by index_open() but closed by index_close() and relation_close() in different places. I also fixed the problem by changing relation_close() to index_close().
Best regards,
==
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/
https://www.highgo.com/
Вложения
Hi, On Fri, Dec 12, 2025 at 04:50:09PM +0800, Chao Li wrote: > Hi Hackers, > > While reading the code of pageinspect, I just found a memory leak > in gist_page_items(): > > ``` > values[4] = CStringGetTextDatum(buf.data); > nulls[4] = false; > ``` > > 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. > This leak is inside a > per-tuple loop, thus it should be fixed. That might be a valid reason though. Do you have an idea of the "leak" size based on the number of tuples? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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 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. -- Michael
Вложения
Hi, On Fri, Dec 12, 2025 at 06:48:09PM +0900, 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? I meant to say: we are probably already in a short lived memory context. I did not mean to say to switch to a new one. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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
> On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote: > If we're going to bother changing this at all, let's consider reusing the buffer. So instead of initStringInfo()+pfreeon every tuple, allocate it once and use resetStringInfo(). +1 -- Daniel Gustafsson
On Dec 12, 2025, at 18:29, Daniel Gustafsson <daniel@yesql.se> wrote:On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka@iki.fi> wrote: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().
+1
It looks like there’s no disagreement on the index_close() fix, while the StringInfo change has triggered some discussion.
So I’ve split the fix into two commits, which I probably should have done from the beginning. 0001 contains the index_close() fix, and 0002 contains the StringInfo fix.
As Heikki suggested, I switched to using resetStringInfo() in 0002. One thing I’d like to highlight is that this function only uses the StringInfo buffer under certain conditions. If we initialize the StringInfo unconditionally, that feels wasteful; if we initialize it lazily, as in 0002, then we need some extra checks to manage its state.