public inbox for [email protected]
help / color / mirror / Atom feedFrom: Baji Shaik <[email protected]>
To: Tristan Partin <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
Date: Fri, 5 Jun 2026 18:44:59 -0500
Message-ID: <CA+fm-RMwUuX7SimgfHZ1o3=RYWQT7wC+KC2+uAgbPqHij25mug@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CA+fm-RMopta1Dmq8udiU5sp+zwTvhUf4+xfbr3rZDfczH+p-xw@mail.gmail.com>
<[email protected]>
On Fri, Jun 5, 2026 at 4:29 PM Tristan Partin <[email protected]> wrote:
> This looks pretty reasonable to me. Nice find. Did you think about
> keeping the code path as is and just removing the first initStringInfo()
> call? Removing the allocation per progress message seems like a good
> idea to me. Maybe you could separate this change into two patches. One
> to fix the memory leak and another to remove the allocation per message.
> A committer could then decide for themselves if the second patch is
> worth committing.
>
Thank you for the review. I hadn't thought of splitting it, but it's
a good idea. I see f1889729dd3 itself is in PG17+, so the bug fix is
a backport candidate independently of the PG19 caller bb8dff9995f.
Patches attached:
0001: drop the redundant initStringInfo() call (backport candidate)
0002: allocate the static buffer once per process via
pq_beginmessage_reuse / pq_endmessage_reuse, to avoid the
per-call allocation (master only)
Thanks,
Baji Shaik
Attachments:
[application/octet-stream] 0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch (1.5K, 3-0001-Fix-memory-leak-in-pgstat_progress_parallel_incr_par.patch)
download | inline diff:
From c7972bc8ee34ec111440bf948c0cebe476fd8cdb Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Fri, 5 Jun 2026 18:41:47 -0500
Subject: [PATCH 1/2] Fix memory leak in pgstat_progress_parallel_incr_param()
When called from a parallel worker, pgstat_progress_parallel_incr_param()
calls initStringInfo() on a static StringInfoData and then immediately
calls pq_beginmessage(), which calls initStringInfo() again. The second
call overwrites buf->data with a freshly palloc'd buffer, orphaning the
first one. pq_endmessage() then frees only the second buffer, so each
call leaks ~1 kB into the per-worker memory context.
Fix by removing the redundant initStringInfo() call.
Oversight of f1889729dd3 ("Add new parallel message type to progress
reporting").
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index b0359771de5..6d2049105ab 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -100,8 +100,6 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
{
static StringInfoData progress_message;
- initStringInfo(&progress_message);
-
pq_beginmessage(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
--
2.50.1 (Apple Git-155)
[application/octet-stream] 0002-Allocate-progress-message-buffer-once-per-parallel-w.patch (2.1K, 4-0002-Allocate-progress-message-buffer-once-per-parallel-w.patch)
download | inline diff:
From 94e1cd63954616f81953950aaf712fa2acf78913 Mon Sep 17 00:00:00 2001
From: Baji Shaik <[email protected]>
Date: Fri, 5 Jun 2026 18:42:08 -0500
Subject: [PATCH 2/2] Allocate progress message buffer once per parallel worker
The static StringInfoData declared in pgstat_progress_parallel_incr_param()
was clearly intended to be allocated once per process and reused across
calls, but the buggy double-initStringInfo() pattern (fixed in the
previous commit) defeated that intent: each call still allocated and
freed a fresh buffer via pq_beginmessage() / pq_endmessage().
Restore the original intent by initializing the static buffer only on
the first call and switching to pq_beginmessage_reuse() and
pq_endmessage_reuse(), which reset and reuse the existing buffer
without (re)allocating.
Author: Baji Shaik <[email protected]>
---
src/backend/utils/activity/backend_progress.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/activity/backend_progress.c b/src/backend/utils/activity/backend_progress.c
index 6d2049105ab..0483741a80e 100644
--- a/src/backend/utils/activity/backend_progress.c
+++ b/src/backend/utils/activity/backend_progress.c
@@ -99,11 +99,23 @@ pgstat_progress_parallel_incr_param(int index, int64 incr)
if (IsParallelWorker())
{
static StringInfoData progress_message;
-
- pq_beginmessage(&progress_message, PqMsg_Progress);
+ static bool progress_message_initialized = false;
+
+ /*
+ * Initialize the message buffer once per process; pq_beginmessage_reuse()
+ * and pq_endmessage_reuse() reset and reuse it on each call to avoid
+ * palloc overhead.
+ */
+ if (!progress_message_initialized)
+ {
+ initStringInfo(&progress_message);
+ progress_message_initialized = true;
+ }
+
+ pq_beginmessage_reuse(&progress_message, PqMsg_Progress);
pq_sendint32(&progress_message, index);
pq_sendint64(&progress_message, incr);
- pq_endmessage(&progress_message);
+ pq_endmessage_reuse(&progress_message);
}
else
pgstat_progress_incr_param(index, incr);
--
2.50.1 (Apple Git-155)
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Fix memory leak in pgstat_progress_parallel_incr_param()
In-Reply-To: <CA+fm-RMwUuX7SimgfHZ1o3=RYWQT7wC+KC2+uAgbPqHij25mug@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox