Fix memory leak in gist_page_items() of pageinspect

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()+pfree on 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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:

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.

While passing through, I have applied 0001. (Note that the email
address you have added to the commit message of 0001 did not match
with your actual email address, perhaps you did not intend to do
that..)
--
Michael

On Dec 15, 2025, at 12:08, Michael Paquier <michael@paquier.xyz> wrote:

(Note that the email
address you have added to the commit message of 0001 did not match
with your actual email address, perhaps you did not intend to do
that..)

Hi Michael,

Yes, that was intentional. lic@highgo.com is my company email address, and I’m supposed to use it when participating in community activities. However, it doesn’t work well with the Mail Archive, so I have to use my personal Gmail address there. That said, for patches that I initiate, I intentionally use my company email address.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:

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

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.

I think that 0002 works fine.

I'm just wondering if:

+ StringInfoData buf = {0}; /* mark as uninitialized */

does not "break" the semantic of "maxlen == 0" means "read-only".

According to the StringInfoData definition comments:

"
* As a special case, a StringInfoData can be initialized with a read-only
* string buffer. In this case "data" does not necessarily point at a
* palloc'd chunk, and management of the buffer storage is the caller's
* responsibility. maxlen is set to zero to indicate that this is the case.
* Read-only StringInfoDatas cannot be appended to or reset.
* Also, it is caller's option whether a read-only string buffer has a
* terminating '\0' or not. This depends on the intended usage."

The patch uses maxlen == 0 to detect "uninitialized", but the documentation
explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a bit or
add a buf_initialized bool in gist_page_items instead?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Dec 15, 2025 at 4:12 PM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:

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

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.

I think that 0002 works fine.

I'm just wondering if:

+ StringInfoData buf = {0}; /* mark as uninitialized */

does not "break" the semantic of "maxlen == 0" means "read-only".

According to the StringInfoData definition comments:

"
* As a special case, a StringInfoData can be initialized with a read-only
* string buffer. In this case "data" does not necessarily point at a
* palloc'd chunk, and management of the buffer storage is the caller's
* responsibility. maxlen is set to zero to indicate that this is the
case.
* Read-only StringInfoDatas cannot be appended to or reset.
* Also, it is caller's option whether a read-only string buffer has a
* terminating '\0' or not. This depends on the intended usage."

The patch uses maxlen == 0 to detect "uninitialized", but the documentation
explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a
bit or
add a buf_initialized bool in gist_page_items instead?

Hi Bertrand,

I think your point is valid. Let's not break the current meaning of maxlen.
I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now becomes
v3-0001.

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

On Mon, Dec 15, 2025 at 5:28 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Mon, Dec 15, 2025 at 4:12 PM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Sat, Dec 13, 2025 at 10:37:40AM +0800, Chao Li wrote:

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

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.

I think that 0002 works fine.

I'm just wondering if:

+ StringInfoData buf = {0}; /* mark as uninitialized */

does not "break" the semantic of "maxlen == 0" means "read-only".

According to the StringInfoData definition comments:

"
* As a special case, a StringInfoData can be initialized with a read-only
* string buffer. In this case "data" does not necessarily point at a
* palloc'd chunk, and management of the buffer storage is the caller's
* responsibility. maxlen is set to zero to indicate that this is the
case.
* Read-only StringInfoDatas cannot be appended to or reset.
* Also, it is caller's option whether a read-only string buffer has a
* terminating '\0' or not. This depends on the intended usage."

The patch uses maxlen == 0 to detect "uninitialized", but the
documentation
explicitly says maxlen == 0 indicates "read-only". Maybe adjust the doc a
bit or
add a buf_initialized bool in gist_page_items instead?

Hi Bertrand,

I think your point is valid. Let's not break the current meaning of
maxlen. I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now
becomes v3-0001.

Oops, forgot the attachment. Here comes it.

Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/

Hi,

On Mon, Dec 15, 2025 at 06:56:06PM +0800, Chao Li wrote:

On Mon, Dec 15, 2025 at 5:28 PM Chao Li <li.evan.chao@gmail.com> wrote:

I think your point is valid. Let's not break the current meaning of
maxlen. I added buf_initialized in v3. As 0001 has been pushed, v2-0002 now
becomes v3-0001.

Oops, forgot the attachment. Here comes it.

LGTM, thanks!

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Mon, Dec 15, 2025 at 01:08:44PM +0900, Michael Paquier wrote:

While passing through, I have applied 0001.

Out of curiosity, I searched for other mismatched index_open/relation_close pairs
in the tree and found a few more with the help of [1]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/mismatched_open_close_pairs.cocci.

They are fixed in the attached.

Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
changed instead. For those we keep the IS_INDEX() checks to reject partitioned
indexes (which index_open() accepts via validate_relation_kind()). So, that also
changes the error messages in some tests. If we do prefer the previous error
messages we could change the close calls instead (I prefer the way it's done
in the attached though).

Thoughts?

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/mismatched_open_close_pairs.cocci

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:

Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
changed instead. For those we keep the IS_INDEX() checks to reject partitioned
indexes (which index_open() accepts via validate_relation_kind()). So, that also
changes the error messages in some tests. If we do prefer the previous error
messages we could change the close calls instead (I prefer the way it's done
in the attached though).

I have noticed that the two surrounding relation_close() calls for the
parent tables did not get the notice of the change for brin.c of what
you are doing for the indexes, while we use table_open(). I have
fixed these.

It would be nicer if IS_INDEX() could be removed in the other code
paths you are suggesting to change, but the partitioned index argument
also means that we would have two code paths in charge of a relkind
check instead of one. Just using relation_*() may be cleaner.
--
Michael

Hi,

On Fri, Dec 19, 2025 at 08:01:54AM +0900, Michael Paquier wrote:

On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:

Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
changed instead. For those we keep the IS_INDEX() checks to reject partitioned
indexes (which index_open() accepts via validate_relation_kind()). So, that also
changes the error messages in some tests. If we do prefer the previous error
messages we could change the close calls instead (I prefer the way it's done
in the attached though).

I have noticed that the two surrounding relation_close() calls for the
parent tables did not get the notice of the change for brin.c of what
you are doing for the indexes, while we use table_open(). I have
fixed these.

Nice catch, thanks!

It would be nicer if IS_INDEX() could be removed in the other code
paths you are suggesting to change, but the partitioned index argument
also means that we would have two code paths in charge of a relkind
check instead of one. Just using relation_*() may be cleaner.

Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
still mean 2 code paths. So, v2 changes the close calls (and that's consistent
with what pgstatginindex_internal() is doing.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Fri, Dec 19, 2025 at 08:01:54AM +0900, Michael Paquier wrote:

On Wed, Dec 17, 2025 at 11:57:13AM +0000, Bertrand Drouvot wrote:

Please note that for hash_bitmap_info() and pgstathashindex() the open calls are
changed instead. For those we keep the IS_INDEX() checks to reject partitioned
indexes (which index_open() accepts via validate_relation_kind()). So, that also
changes the error messages in some tests. If we do prefer the previous error
messages we could change the close calls instead (I prefer the way it's done
in the attached though).

I have noticed that the two surrounding relation_close() calls for the
parent tables did not get the notice of the change for brin.c of what
you are doing for the indexes, while we use table_open(). I have
fixed these.

Nice catch, thanks!

It would be nicer if IS_INDEX() could be removed in the other code
paths you are suggesting to change, but the partitioned index argument
also means that we would have two code paths in charge of a relkind
check instead of one. Just using relation_*() may be cleaner.

Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
still mean 2 code paths. So, v2 changes the close calls (and that's consistent
with what pgstatginindex_internal() is doing.

It would be reasonable to add a comment explaining the choice of
relation_open()/relation_close() instead of the index-specific
index_open()/index_close().

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Hi,

On Fri, Dec 19, 2025 at 02:21:40PM +0800, Japin Li wrote:

On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:

Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
still mean 2 code paths. So, v2 changes the close calls (and that's consistent
with what pgstatginindex_internal() is doing.

It would be reasonable to add a comment explaining the choice of
relation_open()/relation_close() instead of the index-specific
index_open()/index_close().

Yeah that would not hurt. What about before the relation_open() calls?

"
Use relation_open() and not index_open() to avoid the validate_relation_kind()
check as we handle relation validation separately below.
"

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, 19 Dec 2025 at 08:23, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Fri, Dec 19, 2025 at 02:21:40PM +0800, Japin Li wrote:

On Fri, 19 Dec 2025 at 04:29, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:

Yeah, and removing IS_INDEX() and adding a check for partitioned indexes would
still mean 2 code paths. So, v2 changes the close calls (and that's consistent
with what pgstatginindex_internal() is doing.

It would be reasonable to add a comment explaining the choice of
relation_open()/relation_close() instead of the index-specific
index_open()/index_close().

Yeah that would not hurt. What about before the relation_open() calls?

"
Use relation_open() and not index_open() to avoid the validate_relation_kind()
check as we handle relation validation separately below.
"

LGTM.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Hi,

On Fri, Dec 19, 2025 at 05:36:51PM +0800, Japin Li wrote:

On Fri, 19 Dec 2025 at 08:23, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:

Yeah that would not hurt. What about before the relation_open() calls?

"
Use relation_open() and not index_open() to avoid the validate_relation_kind()
check as we handle relation validation separately below.
"

LGTM.

Thanks! Done that way in the attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com