Re: Fix memory leak in gist_page_items() of pageinspect

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

>
> On Mon, Dec 15, 2025 at 4:12 PM Bertrand Drouvot <
> bertranddrouvot(dot)pg(at)gmail(dot)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(at)yesql(dot)se> wrote:
>> >
>> > On 12 Dec 2025, at 11:27, Heikki Linnakangas <hlinnaka(at)iki(dot)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/