| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
| Cc: | Daniel Gustafsson <daniel(at)yesql(dot)se>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Michael Paquier <michael(at)paquier(dot)xyz>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Fix memory leak in gist_page_items() of pageinspect |
| Date: | 2025-12-15 09:28:45 |
| Message-ID: | CAEoWx2=MCJ35VNqmVe1vXK9s4PZ+bN1Xp263UHjfd_A64bwT+w@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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.
Best regards,
Chao Li (Evan)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Soumya S Murali | 2025-12-15 09:37:00 | Re: Checkpointer write combining |
| Previous Message | Dilip Kumar | 2025-12-15 09:25:18 | Re: Proposal: Conflict log history table for Logical Replication |