Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.300.41.1.7\))
Subject: Re: pg_upgrade: fix memory leak in SLRU I/O code
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: <20260205105433.9fcc5f980241bb35a6517448@sraoss.co.jp>
Date: Thu, 5 Feb 2026 12:02:51 +0800
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E05B5ACC-703B-463C-80AB-E63EB2BD75BA@gmail.com>
References: <5DED17B4-D948-4C0B-9DE1-A915D0BFAA54@gmail.com>
 <20260205105433.9fcc5f980241bb35a6517448@sraoss.co.jp>
To: Yugo Nagata <nagata@sraoss.co.jp>
Archived-At: <https://www.postgresql.org/message-id/E05B5ACC-703B-463C-80AB-E63EB2BD75BA%40gmail.com>
Precedence: bulk


> On Feb 5, 2026, at 09:54, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>=20
> On Wed, 4 Feb 2026 17:20:49 +0800
> Chao Li <li.evan.chao@gmail.com> wrote:
>=20
>> Hi Hackers,
>>=20
>> This comes from a previous review and has been on my to-do list for a =
while.
>>=20
>> Since src/bin/pg_upgrade/slru_io.c includes postgres_fe.h, it is =
frontend code, so backend memory contexts are not used here.
>>=20
>> In the current code:
>> ```
>> void
>> FreeSlruWrite(SlruSegState *state)
>> {
>> Assert(state->writing);
>>=20
>> SlruFlush(state);
>>=20
>> if (state->fd !=3D -1)
>> close(state->fd);
>> pg_free(state);
>> }
>> ```
>>=20
>> the SlruSegState itself is freed, but state->dir and state->fn are =
not, which results in a memory leak during pg_upgrade runs. More =
generally, I don=E2=80=99t see a reason to free an object itself without =
also freeing the memory owned by its members.
>=20
> As far as I know, AllocSlruSegState() and FreeSlruWrite() are not =
called
> repeatedly in a loop, and the amount of memory involved is small, so =
the
> impact of the leak seems limited. That said, I agree that it is better =
to
> also free the memory owned by the struct members.
>=20
> Regards,
> Yugo Nagata
>=20

Hi Yugo-san,

Thank you very much for the review. Yes, the leak itself is tiny, the =
main point for me is that the memory owned by the struct members is not =
freed, which can be a bit confusing for code readers. As for the change =
from pstrdup() to pg_strdup(), that felt too trivial to justify a =
separate patch, so I took this as an opportunity to clean it up at the =
same time.

BTW, this is the CF entry: =
https://commitfest.postgresql.org/patch/6462/, you may mark yourself as =
a reviewer.

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