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 Oct 9, 2025, at 22:58, Nathan Bossart <nathandbossart@gmail.com> wrote:
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,
pstrdup() allocates memory from current context for dest string, so the memory it returns should be free-ed. LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
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)