Thread

Re: Fix memory leak in gist_page_items() of pageinspect

Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-12-15T08:12:28Z

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