Обсуждение: memory leak in dbase_redo()

Hi,

I was trying to fix an independent problem reported by skink (caused by me)
and valgrind greeted me with the following complaint during a crash-restart:

==350002== 11 bytes in 1 blocks are definitely lost in loss record 19 of 121
==350002==    at 0xFD8995: MemoryContextAlloc (mcxt.c:1250)
==350002==    by 0xFD9C38: MemoryContextStrdup (mcxt.c:1751)
==350002==    by 0xFD9C7F: pstrdup (mcxt.c:1761)
==350002==    by 0xA184BF: dbase_redo (dbcommands.c:3375)
==350002==    by 0x97BD72: ApplyWalRecord (xlogrecovery.c:2002)
==350002==    by 0x97B879: PerformWalRecovery (xlogrecovery.c:1832)
==350002==    by 0x96B377: StartupXLOG (xlog.c:5894)
==350002==    by 0xCBD29F: StartupProcessMain (startup.c:258)
==350002==    by 0xCB4B63: postmaster_child_launch (launch_backend.c:268)
==350002==    by 0xCBC369: StartChildProcess (postmaster.c:3983)
==350002==    by 0xCB86BA: PostmasterMain (postmaster.c:1396)
==350002==    by 0xB84BF5: main (main.c:231)

And I think it is right. XLOG_DBASE_CREATE_FILE_COPY is careful to
pfree(parent_path), but XLOG_DBASE_CREATE_WAL_LOG isn't.

This isn't a particularly bad leak, given the amounts of data involved and the
cost of the underlying operation, yet it still seems like somethin we ought to
fix.

Greetings,

Andres Freund

On Thu, Oct 09, 2025 at 08:08:05AM -0400, Andres Freund wrote:
> And I think it is right. XLOG_DBASE_CREATE_FILE_COPY is careful to
> pfree(parent_path), but XLOG_DBASE_CREATE_WAL_LOG isn't.

It looks like this was introduced by commit 9e4f914, which was
back-patched, but the code path in question first appears in v15.  So,
presumably something like the following needs to be back-patched that far.
I can take care of it unless someone else wants it.

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 2793fd83771..4d65e8c46c2 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -3375,6 +3375,7 @@ dbase_redo(XLogReaderState *record)
         parent_path = pstrdup(dbpath);
         get_parent_directory(parent_path);
         recovery_create_dbdir(parent_path, true);
+        pfree(parent_path);

         /* Create the database directory with the version file. */
         CreateDirAndVersionFile(dbpath, xlrec->db_id, xlrec->tablespace_id,

-- 
nathan

On 2025-Oct-09, Nathan Bossart wrote:

> It looks like this was introduced by commit 9e4f914, which was
> back-patched, but the code path in question first appears in v15.  So,
> presumably something like the following needs to be back-patched that far.
> I can take care of it unless someone else wants it.

Hmm, it's my bug, please let me get it done.

> diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
> index 2793fd83771..4d65e8c46c2 100644
> --- a/src/backend/commands/dbcommands.c
> +++ b/src/backend/commands/dbcommands.c
> @@ -3375,6 +3375,7 @@ dbase_redo(XLogReaderState *record)
>          parent_path = pstrdup(dbpath);
>          get_parent_directory(parent_path);
>          recovery_create_dbdir(parent_path, true);
> +        pfree(parent_path);

Yeah, this LGTM.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Thu, Oct 09, 2025 at 06:36:58PM +0200, Álvaro Herrera wrote:
> On 2025-Oct-09, Nathan Bossart wrote:
>> It looks like this was introduced by commit 9e4f914, which was
>> back-patched, but the code path in question first appears in v15.  So,
>> presumably something like the following needs to be back-patched that far.
>> I can take care of it unless someone else wants it.
> 
> Hmm, it's my bug, please let me get it done.

Sounds good, thanks.

-- 
nathan

On 2025-Oct-09, Nathan Bossart wrote:

> On Thu, Oct 09, 2025 at 06:36:58PM +0200, Álvaro Herrera wrote:
> > On 2025-Oct-09, Nathan Bossart wrote:
> >> It looks like this was introduced by commit 9e4f914, which was
> >> back-patched, but the code path in question first appears in v15.  So,
> >> presumably something like the following needs to be back-patched that far.
> >> I can take care of it unless someone else wants it.
> > 
> > Hmm, it's my bug, please let me get it done.
> 
> Sounds good, thanks.

Pushed now, thanks.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."                              (Brian Kernighan)