pg_upgrade: fix memory leak in SLRU I/O code

On Wed, 4 Feb 2026 17:20:49 +0800
Chao Li <li.evan.chao@gmail.com> wrote:

Hi Hackers,

This comes from a previous review and has been on my to-do list for a while.

Since src/bin/pg_upgrade/slru_io.c includes postgres_fe.h, it is frontend code, so backend memory contexts are not used here.

In the current code:
```
void
FreeSlruWrite(SlruSegState *state)
{
Assert(state->writing);

SlruFlush(state);

if (state->fd != -1)
close(state->fd);
pg_free(state);
}
```

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’t see a reason to free an object itself without also freeing the memory owned by its members.

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.

Regards,
Yugo Nagata

While looking at this, I also noticed that state->dir is allocated using pstrdup(). To better align with frontend conventions, the patch switches this to pg_strdup() and introduces a common cleanup helper to free all resources associated with SlruSegState.

See the attached patch.

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

--
Yugo Nagata <nagata@sraoss.co.jp>

On Wed, Feb 4, 2026 at 2:51 PM Chao Li <li.evan.chao@gmail.com> wrote:

Hi Hackers,

This comes from a previous review and has been on my to-do list for a while.

Since src/bin/pg_upgrade/slru_io.c includes postgres_fe.h, it is frontend code, so backend memory contexts are not used here.

In the current code:
```
void
FreeSlruWrite(SlruSegState *state)
{
Assert(state->writing);

SlruFlush(state);

if (state->fd != -1)
close(state->fd);
pg_free(state);
}
```

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’t see a reason to free an object itself without also freeing the memory owned by its members.

While looking at this, I also noticed that state->dir is allocated using pstrdup(). To better align with frontend conventions, the patch switches this to pg_strdup() and introduces a common cleanup helper to free all resources associated with SlruSegState.

See the attached patch.

The patch LGTM, just one suggestion, adds a one liner comment over the
new function[1]+static void +FreeSlruSegState(SlruSegState *state) +{ + if (state->fd != -1) + close(state->fd); + + pg_free(state->fn); + pg_free(state->dir); + pg_free(state); +} for consistency.

[1]
+static void
+FreeSlruSegState(SlruSegState *state)
+{
+ if (state->fd != -1)
+ close(state->fd);
+
+ pg_free(state->fn);
+ pg_free(state->dir);
+ pg_free(state);
+}

--
Regards,
Dilip Kumar
Google

On Feb 5, 2026, at 09:54, Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Wed, 4 Feb 2026 17:20:49 +0800
Chao Li <li.evan.chao@gmail.com> wrote:

Hi Hackers,

This comes from a previous review and has been on my to-do list for a while.

Since src/bin/pg_upgrade/slru_io.c includes postgres_fe.h, it is frontend code, so backend memory contexts are not used here.

In the current code:
```
void
FreeSlruWrite(SlruSegState *state)
{
Assert(state->writing);

SlruFlush(state);

if (state->fd != -1)
close(state->fd);
pg_free(state);
}
```

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’t see a reason to free an object itself without also freeing the memory owned by its members.

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.

Regards,
Yugo Nagata

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/

On Thu, Feb 05, 2026 at 12:02:51PM +0800, Chao Li wrote:

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.

Does it really matter memory-wise? Even in the case of state->fn,
SlruReadSwitchPageSlow() and SlruWriteSwitchPageSlow() make sure to
free it before switching to a new segment to process, and these state
allocations are done once. Well, okay, twice as of the members *and*
the offsets, but the logic as written is not going to bloat memory
with a short loop execution.

In short, I see nothing worth changing.
--
Michael

On Feb 5, 2026, at 12:37, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 05, 2026 at 12:02:51PM +0800, Chao Li wrote:

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.

Does it really matter memory-wise? Even in the case of state->fn,
SlruReadSwitchPageSlow() and SlruWriteSwitchPageSlow() make sure to
free it before switching to a new segment to process, and these state
allocations are done once. Well, okay, twice as of the members *and*
the offsets, but the logic as written is not going to bloat memory
with a short loop execution.

In short, I see nothing worth changing.
--
Michael

Hi Michael,

I agree that memory bloat is not really the issue here. If the code never freed the state at all, I wouldn’t have bothered with this patch.

My concern is more about the pattern: freeing the container structure while leaving its members allocated. That feels inconsistent and potentially confusing to readers. Either owning objects should be fully freed, or not freed at all, but partial freeing doesn’t seem like a great precedent. I’m not sure that’s a pattern we want to encourage in PG code.

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

On Thu, Feb 05, 2026 at 01:02:08PM +0800, Chao Li wrote:

My concern is more about the pattern: freeing the container
structure while leaving its members allocated. That feels
inconsistent and potentially confusing to readers. Either owning
objects should be fully freed, or not freed at all, but partial
freeing doesn’t seem like a great precedent. I’m not sure that’s a
pattern we want to encourage in PG code.

For one-time allocations that are freed once we exit the binary, I
would not have bothered about freeing the state, TBH. Closing the fd
in these two paths is a good thing, though.
--
Michael

On Feb 5, 2026, at 13:28, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Feb 05, 2026 at 01:02:08PM +0800, Chao Li wrote:

My concern is more about the pattern: freeing the container
structure while leaving its members allocated. That feels
inconsistent and potentially confusing to readers. Either owning
objects should be fully freed, or not freed at all, but partial
freeing doesn’t seem like a great precedent. I’m not sure that’s a
pattern we want to encourage in PG code.

For one-time allocations that are freed once we exit the binary, I
would not have bothered about freeing the state,

Exactly. In this case, memory leaking is not a problem at all. My thinking was that once we do decide to free the top-level object, it feels more consistent and less surprising to also free what it owns, even if the lifetime is effectively the whole process. It’s less about resource pressure and more about keeping the ownership model clear for future readers and maintenance.

TBH, I’ve been very careful about judging whether a memory-related issue is really worth patching, as I asked you about this before. I think I used the wrong subject line here, the point isn’t about a memory leak at all.

I understand the trade-off, and I’m happy to drop this if the consensus is that it’s not worth touching.

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

On 2026-Feb-05, Chao Li wrote:

Exactly. In this case, memory leaking is not a problem at all. My
thinking was that once we do decide to free the top-level object, it
feels more consistent and less surprising to also free what it owns,
even if the lifetime is effectively the whole process. It’s less about
resource pressure and more about keeping the ownership model clear for
future readers and maintenance.

Maybe we should just change the "pg_free(state)" to "/* no point freeing
memory */" or something like that.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"We're here to devour each other alive" (Hobbes)